[#1757] feat(server): Add block number check on getting shuffle result #1758

zuston · 2024-05-30T06:43:20Z

What changes were proposed in this pull request?

Add block number check on getting shuffle result

Why are the changes needed?

Data validation, ensure data stable and correct

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

… result

github-actions · 2024-05-30T07:03:27Z

Test Results

2 433 files +8 2 433 suites +8 5h 0m 9s ⏱️ + 3m 32s
934 tests ±0 933 ✅ +1 1 💤 ±0 0 ❌ ±0
10 828 runs +8 10 814 ✅ +9 14 💤 ±0 0 ❌ ±0

Results for commit 1c7908e. ± Comparison against base commit d182a03.

♻️ This comment has been updated with latest results.

server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java

rickyma · 2024-05-30T07:03:11Z

server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java

@@ -198,6 +201,25 @@ public Set<Integer> getShuffleIds() {
    return partitionDataSizes.keySet();
  }

+  public void incrBlockNumber(int shuffleId, int partitionId) {


incrBlockNumber -> incBlockNumber

This should be a better naming.

EnricoMi

In which scenario do you expect the block number to disalign? What are you trying to guard against here? One side we add block ids and on the other side we retrieve block ids. You suspect incorrect usage of the API by the client or a bug server side? Sounds like that should be guarded against by testing so we are confident, not in production runtime.

server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java

zuston · 2024-05-30T08:10:19Z

In which scenario do you expect the block number to disalign? What are you trying to guard against here? One side we add block ids and on the other side we retrieve block ids. You suspect incorrect usage of the API by the client or a bug server side? Sounds like that should be guarded against by testing so we are confident, not in production runtime.

We suspect the race condition may exist on the process of reporting shuffle result, which may cause the blockId miss. But this is not determinzed, and so I want to introduce the extra blockId counter check for every partition. Anyway, fast fail is better than the silcent incorrect succeed.

jerqi · 2024-05-30T08:36:46Z

Maybe addFinishedBlockIds can return int. We can add some logs if we receive some duplicated blocks.

zuston · 2024-05-30T11:00:37Z

Maybe addFinishedBlockIds can return int. We can add some logs if we receive some duplicated blocks.

Yes. Let me optimize this part tomorrow.

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java

rickyma · 2024-05-31T03:39:59Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java

@@ -506,14 +506,24 @@ public void reportShuffleResult(
        "appId[" + appId + "], shuffleId[" + shuffleId + "], taskAttemptId[" + taskAttemptId + "]";

    try {
+      int expected =
+          partitionToBlockIds.values().stream().map(x -> x.length).reduce(0, (a, b) -> a + b);


This is more readable:
int expected = partitionToBlockIds.values().stream().mapToInt(x -> x.length).sum();

Sounds good.

rickyma · 2024-05-31T03:41:00Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java

@@ -506,14 +506,24 @@ public void reportShuffleResult(
        "appId[" + appId + "], shuffleId[" + shuffleId + "], taskAttemptId[" + taskAttemptId + "]";

    try {
+      int expected =


Maybe expectedNum and updatedNum?

Needn't. The type has indicated everything.

rickyma · 2024-05-31T03:45:55Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java

-      shuffleServer
-          .getShuffleTaskManager()
-          .addFinishedBlockIds(appId, shuffleId, partitionToBlockIds, bitmapNum);
+          "Accepted {} blockIds report of {} partitions as shuffle result for the task of {}",


I think the log could be more readable:

LOG.info( "Accepted blockIds report for {} blocks across {} partitions as shuffle result for task {}", expectedNum, partitionToBlockIds.size(), request);

Sounds good.

jerqi · 2024-05-31T07:19:22Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java

+          expected,
+          partitionToBlockIds.size(),
+          request);
+      int updated =


updated -> updatedBlockCount

jerqi · 2024-05-31T07:19:48Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java

+          shuffleServer
+              .getShuffleTaskManager()
+              .addFinishedBlockIds(appId, shuffleId, partitionToBlockIds, bitmapNum);
+      if (expected != updated) {


expected -> expectedBlockCount

rickyma · 2024-05-31T07:56:07Z

server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java

-          bitmap.addLong(blockId);
+          if (!bitmap.contains(blockId)) {
+            bitmap.addLong(blockId);
+            updated++;


Refer to @jerqi 's suggestion, you may also rename this:
updatedBlockCount
totalUpdatedBlockCount

jerqi

LGTM.

zuston · 2024-06-03T02:14:34Z

Thanks everyone for your review. Merged.

…ng shuffle result apache#1758 LINK: apache#1758

maobaolong

@zuston I have couple of question inline, would you please take a look?

maobaolong · 2024-08-06T01:51:57Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java

-      shuffleServer
-          .getShuffleTaskManager()
-          .addFinishedBlockIds(appId, shuffleId, partitionToBlockIds, bitmapNum);
+          "Accepted blockIds report for {} blocks across {} partitions as shuffle result for task {}",


This log is verbose and fill server log, is there a better way to reduce it or do you think DEBUG level can be better?

maobaolong · 2024-08-06T01:56:20Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java

+              .addFinishedBlockIds(appId, shuffleId, partitionToBlockIds, bitmapNum);
+      if (expectedBlockCount != updatedBlockCount) {
+        LOG.warn(
+            "Existing {} duplicated blockIds on blockId report for appId: {}, shuffleId: {}",


Unfortunately, we encountered this warn level log these days, it make us stressed.

Is there indicate something wrong? Can you explain what rare condition scenario can make updatedBlockCount less than expectedBlockCount?

Why there are duplicated blockIds on blockId report?

[apache#1757] feat(server): Add block number check on getting shuffle…

c2a1a8f

… result

rickyma reviewed May 30, 2024

View reviewed changes

fix

db0e92c

zuston force-pushed the threadSafe branch from da63e43 to db0e92c Compare May 30, 2024 07:07

EnricoMi reviewed May 30, 2024

View reviewed changes

server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java Outdated Show resolved Hide resolved

zuston added 2 commits May 30, 2024 15:59

fix

9193803

fix

673652d

zuston added 2 commits May 31, 2024 10:09

fix

8e8c3d6

output duplicated blockId number in log

b82f3e4

jerqi reviewed May 31, 2024

View reviewed changes

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java Outdated Show resolved Hide resolved

fix

07d9668

rickyma reviewed May 31, 2024

View reviewed changes

optimize

c8ad64d

jerqi reviewed May 31, 2024

View reviewed changes

rickyma reviewed May 31, 2024

View reviewed changes

fix

1c7908e

jerqi approved these changes May 31, 2024

View reviewed changes

zuston merged commit fd64d9d into apache:master Jun 3, 2024
41 checks passed

zuston added a commit to zuston/incubator-uniffle that referenced this pull request Jun 3, 2024

BACKPORT: [apache#1757] feat(server): Add block number check on getti…

f4ce4de

…ng shuffle result apache#1758 LINK: apache#1758

maobaolong reviewed Aug 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1757] feat(server): Add block number check on getting shuffle result #1758

[#1757] feat(server): Add block number check on getting shuffle result #1758

zuston commented May 30, 2024

github-actions bot commented May 30, 2024 •

edited

Loading

rickyma May 30, 2024

EnricoMi left a comment

zuston commented May 30, 2024

jerqi commented May 30, 2024

zuston commented May 30, 2024

rickyma May 31, 2024

zuston May 31, 2024

rickyma May 31, 2024

zuston May 31, 2024

rickyma May 31, 2024

zuston May 31, 2024

jerqi May 31, 2024

jerqi May 31, 2024

rickyma May 31, 2024

jerqi left a comment •

edited

Loading

zuston commented Jun 3, 2024

maobaolong left a comment •

edited

Loading

maobaolong Aug 6, 2024

maobaolong Aug 6, 2024

[#1757] feat(server): Add block number check on getting shuffle result #1758

[#1757] feat(server): Add block number check on getting shuffle result #1758

Conversation

zuston commented May 30, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

github-actions bot commented May 30, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

EnricoMi left a comment

Choose a reason for hiding this comment

zuston commented May 30, 2024

jerqi commented May 30, 2024

zuston commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi left a comment • edited Loading

Choose a reason for hiding this comment

zuston commented Jun 3, 2024

maobaolong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 30, 2024 •

edited

Loading

jerqi left a comment •

edited

Loading

maobaolong left a comment •

edited

Loading