[#1045] fix(server): shuffle server may hang when restart worker due to multi require-momery and no require-momery release #1058

lifeSo · 2023-07-31T15:19:40Z

What changes were proposed in this pull request?

shuffle server may hang when restart worker due to multi require-momery and no require-momery release.
So, fix the bug

Why are the changes needed?

shuffle server may hang when restart worker due to multi require-momery and no require-momery release.
So, fix the bug

Fix: #1045

Does this PR introduce any user-facing change?

No.

How was this patch tested?

unit test

jerqi · 2023-08-01T02:34:19Z

@zuston Could you help me review this pr?

jerqi · 2023-08-01T02:40:21Z

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

@@ -408,6 +408,10 @@ public long getPartitionDataSize(String appId, int shuffleId, int partitionId) {

  public long requireBuffer(
      String appId, int shuffleId, List<Integer> partitionIds, int requireSize) {
+    boolean isRegistered = shuffleBufferManager.checkIfRegistered(appId, shuffleId, partitionIds);
+    if (!isRegistered) {
+      return -4; // Keep the same with StatusCode


Could give -1 and -4 a error code? like

enum ERRORCODE { NO_BUFFER(-1), NO_REGISTER(-4) }

Or we can reuse the error code which existed.

I find there is no error code existed I can use, and I craete RequireBufferStatusCode

jerqi · 2023-08-01T02:41:37Z

...-tez/src/main/java/org/apache/tez/runtime/library/common/shuffle/impl/RssTezFetcherTask.java

@@ -186,6 +187,7 @@ protected FetchResult callInternal() throws Exception {
              taskIdBitmap,
              new ArrayList<>(serverInfoSet),
              readerJobConf,
+              new TezIdHelper(),


Is this another bug? Will it cause correctness issues?

jerqi · 2023-08-01T02:45:36Z

server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java

@@ -189,6 +189,20 @@ private void updateShuffleSize(String appId, int shuffleId, long size) {
    shuffleIdToSize.get(shuffleId).addAndGet(size);
  }

+  public boolean checkIfRegistered(String appId, int shuffleId, List<Integer> partitionIds) {


Could we use ShuffleTaskManager#shuffleTaskInfos to judge whether is registered?

+1 I have similar thought. The appId + shuffleId check existence are enough. Right?

codecov-commenter · 2023-08-01T16:12:38Z

Codecov Report

Merging #1058 (13f22ff) into master (9ef3747) will increase coverage by 1.09%.
Report is 5 commits behind head on master.
The diff coverage is 70.00%.

@@             Coverage Diff              @@
##             master    #1058      +/-   ##
============================================
+ Coverage     54.18%   55.27%   +1.09%     
+ Complexity     2549     2546       -3     
============================================
  Files           386      367      -19     
  Lines         21861    19537    -2324     
  Branches       1813     1815       +2     
============================================
- Hits          11845    10799    -1046     
+ Misses         9315     8113    -1202     
+ Partials        701      625      -76

Files Changed	Coverage Δ
...library/common/shuffle/impl/RssTezFetcherTask.java	`0.00% <ø> (ø)`
...on/shuffle/orderedgrouped/RssShuffleScheduler.java	`46.87% <ø> (-0.07%)`	⬇️
...ffle/client/impl/grpc/ShuffleServerGrpcClient.java	`0.00% <0.00%> (ø)`
...client/impl/grpc/ShuffleServerGrpcNettyClient.java	`0.00% <0.00%> (ø)`
...pache/uniffle/server/ShuffleServerGrpcService.java	`0.76% <0.00%> (-0.01%)`	⬇️
...ime/library/common/shuffle/impl/RssTezFetcher.java	`77.89% <50.00%> (ø)`
...mon/shuffle/orderedgrouped/RssTezBypassWriter.java	`45.16% <100.00%> (+13.16%)`	⬆️
.../org/apache/uniffle/server/ShuffleTaskManager.java	`76.63% <100.00%> (+0.33%)`	⬆️
...uniffle/server/buffer/RequireBufferStatusCode.java	`100.00% <100.00%> (ø)`

... and 37 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…heck

lifeSo · 2023-08-02T02:24:09Z

How about the spark test code, I did not change the spark test.

jerqi · 2023-08-02T02:26:20Z

How about the spark test code, I did not change the spark test.

But you modify the code shuffle server, Spark integration tests will use shuffle server.

lifeSo · 2023-08-02T02:29:11Z

Ok, may be the spark test not register, and I only changed register logical.

jerqi · 2023-08-02T02:31:22Z

Ok, may be the spark test not register, and I only changed register logical.

Spark register the application in the RssShuffleManager.

lifeSo · 2023-08-04T03:44:37Z

RssShuffleManager

In fact, likes not registered from my test and log or maybe other interface

jerqi · 2023-08-04T03:57:36Z

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

-          ShuffleServerMetrics.counterTotalRequireBufferFailedForHugePartition.inc();
-          return -1;
-        }
+    if (null == shuffleTaskInfo && !"EMPTY".equals(appId)) {


Why do we add EMPTY here? It's weird.

jerqi

LGTM, thanks @lifeSo

jerqi · 2023-08-05T00:09:34Z

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

+    if (null == shuffleTaskInfo) {
+      return RequireBufferStatusCode.NO_REGISTER.statusCode();
+    }
+    for (int partitionId : partitionIds) {


@zuston Could you take another look at this place?

I think the change is right. But I will introduce new grpc status code like BUFFER_LIMIT_FOR_HUGE_PARTITION instead of NO_BUFFER

jerqi · 2023-08-07T08:51:44Z

@zuston Could you help me this pr?

zuston

lgtm.

zuston · 2023-08-08T06:38:07Z

client-tez/src/main/java/org/apache/tez/runtime/library/common/shuffle/impl/RssTezFetcher.java

@@ -133,7 +133,7 @@ public void copyFromRssServer() throws IOException {
        hasPendingData = false;
        uncompressedData = null;
      } else {
-        LOG.info("uncompressedData is null");
+        LOG.info("UncompressedData is null");


I think this is not related with current issue.

Anyway, let's reserve these changes.

zuston · 2023-08-08T06:45:32Z

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

+    if (null == shuffleTaskInfo) {
+      return RequireBufferStatusCode.NO_REGISTER.statusCode();
+    }
+    for (int partitionId : partitionIds) {


I think the change is right. But I will introduce new grpc status code like BUFFER_LIMIT_FOR_HUGE_PARTITION instead of NO_BUFFER

lifeSo added 7 commits July 17, 2023 15:29

增加条件判断，当内存不够时，返回wait MapOutput进入循环等待

3f90743

Merge branch 'apache:master' into master

a5e8c73

requireBuffer 增加register判断

43e82f6

revert

e6a4f4c

使用TezIdHelper，而非默认

e594dc5

code format

8c7079d

code format

0b7fc46

jerqi requested a review from zuston August 1, 2023 01:48

jerqi changed the title ~~shuffle server may hang when restart worker due to multi require-momery and no require-momery release~~ [#1045] fix(server): shuffle server may hang when restart worker due to multi require-momery and no require-momery release Aug 1, 2023

jerqi reviewed Aug 1, 2023

View reviewed changes

lifeSo added 2 commits August 2, 2023 00:02

修改判断register的方法实现

bbf260f

revert commit

e36d916

lifeSo added 3 commits August 2, 2023 00:31

Fix to use TezIdHelper to getTaskAttemptId instead of default IdHelper

736439f

Merge branch 'github-feature-id-helper' into feature-require-buffer-c…

ec939e7

…heck

just format code

cb468c3

lifeSo added 2 commits August 3, 2023 20:22

revert

45404e4

when there is no registe

97be929

jerqi reviewed Aug 4, 2023

View reviewed changes

lifeSo added 2 commits August 5, 2023 02:17

修改测试类，不使用EMPTY的appId

1c42472

format code

13f22ff

jerqi previously approved these changes Aug 5, 2023

View reviewed changes

jerqi reviewed Aug 5, 2023

View reviewed changes

添加No_register的判断，判断未注册，则抛出NotRetryException

7610363

lifeSo dismissed jerqi’s stale review via 7610363 August 7, 2023 08:50

just format code

4c501ec

zuston approved these changes Aug 8, 2023

View reviewed changes

jerqi merged commit ca24531 into apache:master Aug 8, 2023
30 checks passed

zuston mentioned this pull request Aug 8, 2023

fix: strictly verify whether app is registered especially after server restart #1120

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1045] fix(server): shuffle server may hang when restart worker due to multi require-momery and no require-momery release #1058

[#1045] fix(server): shuffle server may hang when restart worker due to multi require-momery and no require-momery release #1058

lifeSo commented Jul 31, 2023 •

edited by jerqi

Loading

jerqi commented Aug 1, 2023

jerqi Aug 1, 2023

lifeSo Aug 1, 2023

jerqi Aug 1, 2023

jerqi Aug 1, 2023

zuston Aug 1, 2023

codecov-commenter commented Aug 1, 2023 •

edited

Loading

lifeSo commented Aug 2, 2023

jerqi commented Aug 2, 2023

lifeSo commented Aug 2, 2023

jerqi commented Aug 2, 2023

lifeSo commented Aug 4, 2023 •

edited

Loading

jerqi Aug 4, 2023

jerqi left a comment

jerqi Aug 5, 2023

zuston Aug 8, 2023

jerqi commented Aug 7, 2023

zuston left a comment

zuston Aug 8, 2023

zuston Aug 8, 2023

[#1045] fix(server): shuffle server may hang when restart worker due to multi require-momery and no require-momery release #1058

[#1045] fix(server): shuffle server may hang when restart worker due to multi require-momery and no require-momery release #1058

Conversation

lifeSo commented Jul 31, 2023 • edited by jerqi Loading

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?

jerqi commented Aug 1, 2023

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

codecov-commenter commented Aug 1, 2023 • edited Loading

Codecov Report

lifeSo commented Aug 2, 2023

jerqi commented Aug 2, 2023

lifeSo commented Aug 2, 2023

jerqi commented Aug 2, 2023

lifeSo commented Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi commented Aug 7, 2023

zuston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifeSo commented Jul 31, 2023 •

edited by jerqi

Loading

codecov-commenter commented Aug 1, 2023 •

edited

Loading

lifeSo commented Aug 4, 2023 •

edited

Loading