Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MINOR] improve(server): Add debug log when cacheShuffleData #2156

Merged
merged 4 commits into from
Oct 12, 2024

Conversation

maobaolong
Copy link
Member

What changes were proposed in this pull request?

Add debug log when cacheShuffleData

Why are the changes needed?

Supply a way to know the cacheShuffleData detail, include the partitionId, blockCount, we can know well from this debug level log.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

No need.

Copy link

github-actions bot commented Sep 27, 2024

Test Results

 2 920 files  ±0   2 920 suites  ±0   5h 56m 53s ⏱️ + 1m 2s
 1 033 tests ±0   1 031 ✅ ±0   2 💤 ±0  0 ❌ ±0 
12 987 runs  ±0  12 957 ✅ ±0  30 💤 ±0  0 ❌ ±0 

Results for commit 97cb719. ± Comparison against base commit 267353e.

♻️ This comment has been updated with latest results.

@@ -71,4 +77,8 @@ public ShufflePartitionedBlock[] getBlockList() {
public long getTotalBlockSize() {
return totalBlockSize;
}

public long getTotalBlockLength() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference of block lenght and block size?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zuston

  public int getSize() {
    return length + 3 * 8 + 2 * 4;
  }

The block size is larger 3 * 8 + 2 * 4 than block length which instead of encoded length of this block.

@jerqi
Copy link
Contributor

jerqi commented Oct 8, 2024

Actually it's weird that we have size and length at the same time. Maybe we should have a better name for them.

@maobaolong
Copy link
Member Author

@jerqi Yes, do you think Encoded length and DataLength should be better?

@maobaolong maobaolong force-pushed the addDebugLogForCacheShuffleData branch from 071d5c5 to 8c4a4a3 Compare October 8, 2024 11:46
@maobaolong maobaolong closed this Oct 9, 2024
@maobaolong maobaolong reopened this Oct 9, 2024
@jerqi
Copy link
Contributor

jerqi commented Oct 9, 2024

@jerqi Yes, do you think Encoded length and DataLength should be better?

Better than now.

@jerqi
Copy link
Contributor

jerqi commented Oct 9, 2024

@jerqi Yes, do you think Encoded length and DataLength should be better?

Encoded length is larger than data length, isn't it?

@maobaolong
Copy link
Member Author

@jerqi Yes, encoded length include data length and data structure size.

@maobaolong maobaolong closed this Oct 9, 2024
@maobaolong maobaolong reopened this Oct 9, 2024
@maobaolong maobaolong closed this Oct 9, 2024
@maobaolong maobaolong reopened this Oct 9, 2024
for (ShufflePartitionedBlock block : this.blockList) {
size += block.getSize();
encodedLength += block.getSize();
Copy link
Contributor

@jerqi jerqi Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we modify the name in the ShufflePartitionedBlock, too?

Copy link
Member Author

@maobaolong maobaolong Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done

@@ -20,31 +20,37 @@
import java.util.Arrays;

import com.google.common.annotations.VisibleForTesting;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.commons.lang3.tuple.Triple;
Copy link
Contributor

@jerqi jerqi Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we extract a class instead of using Tripple?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I abstract a class named ShufflePartitionedBlocksInfo

@maobaolong maobaolong force-pushed the addDebugLogForCacheShuffleData branch from 0d96f95 to 723c602 Compare October 9, 2024 08:07
@maobaolong maobaolong requested a review from jerqi October 9, 2024 08:08
@maobaolong
Copy link
Member Author

@jerqi Thanks for your suggestion, addressed it by the newest commit, PTAL.

jerqi
jerqi previously approved these changes Oct 9, 2024
Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@maobaolong
Copy link
Member Author

@jerqi Thanks for the approved, i push a new commit to avoid introduce new data structure, hope this can be better? WDYT?

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for me.

@jerqi jerqi merged commit 270afaa into apache:master Oct 12, 2024
43 checks passed
maobaolong added a commit to maobaolong/incubator-uniffle that referenced this pull request Nov 4, 2024
…2156)

### What changes were proposed in this pull request?

Add debug log when cacheShuffleData

### Why are the changes needed?

Supply a way to know the cacheShuffleData detail, include the partitionId, blockCount, we can know well from this debug level log. 

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

No need.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants