[#1119] improvement: Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER #1393

lifeSo · 2023-12-23T12:21:58Z

What changes were proposed in this pull request?

Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER

Why are the changes needed?

Fix: # #1119

Does this PR introduce any user-facing change?

No.

How was this patch tested?

test on prd and unit test case

codecov-commenter · 2023-12-23T12:31:26Z

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (44fad8d) 53.23% compared to head (9085751) 54.26%.
Report is 10 commits behind head on master.

Files	Patch %	Lines
...pache/uniffle/server/ShuffleServerGrpcService.java	0.00%	17 Missing ⚠️
...ffle/client/impl/grpc/ShuffleServerGrpcClient.java	0.00%	9 Missing ⚠️
...he/uniffle/common/exception/NoBufferException.java	0.00%	6 Missing ⚠️
...n/exception/NoBufferForHugePartitionException.java	0.00%	6 Missing ⚠️
.../uniffle/common/exception/NoRegisterException.java	0.00%	6 Missing ⚠️
.../org/apache/uniffle/server/ShuffleTaskManager.java	81.81%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1393      +/-   ##
============================================
+ Coverage     53.23%   54.26%   +1.02%     
  Complexity     2720     2720              
============================================
  Files           419      402      -17     
  Lines         23994    21667    -2327     
  Branches       2051     2043       -8     
============================================
- Hits          12774    11758    -1016     
+ Misses        10427     9187    -1240     
+ Partials        793      722      -71

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…re_1119_new_2023_12_23 * feature_1119_BUFFER_LIMIT_OF_HUGE_PARTITION: add file for commit [apache#1189] feat(server): Add netty used direct memory size metric (apache#1363) format format format rename file rename [apache#960] fix(dashboard): simplify dependency and correct the startup script (apache#1347) [apache#1348] improvement(metrics): Unify tags generation for shuffle-server metrics reporter (apache#1349) [MINOR] chore: fix kubernetes ci pipeline (apache#1368) [MINOR] fix(spark): Fix NPE for ShuffleWriteClientImpl.unregisterShuffle (apache#1367) format code format code 添加堆外内存监控的代码

zuston · 2023-12-28T11:20:20Z

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

    ShuffleTaskInfo shuffleTaskInfo = shuffleTaskInfos.get(appId);
    if (null == shuffleTaskInfo) {
-      return RequireBufferStatusCode.NO_REGISTER.statusCode();
+      LOG.error("Find not registered app, appId: {}, shuffleId: {}", appId, shuffleId);


How about no such app is registered. appId: {}, shuffleId: {}

zuston · 2023-12-28T11:22:39Z

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

          appId, shuffleId, partitionId, partitionUsedDataSize)) {
        ShuffleServerMetrics.counterTotalRequireBufferFailedForHugePartition.inc();
-        return RequireBufferStatusCode.NO_BUFFER.statusCode();
+        String errorMessage =


Why not throwing the NoBufferForHugePartitionException ?

zuston · 2023-12-28T11:23:29Z

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

-        return RequireBufferStatusCode.NO_BUFFER.statusCode();
+        String errorMessage =
+            String.format(
+                "No Buffer For Huge Partition, appId: %s, shuffleId: %s, partitionIds: %s, partitionUsedDataSize: %s",


How about Huge partition is limited to writing. appId: %s, shuffleId: %s, partitionIds: %s, partitionUsedDataSize: %s ?

zuston · 2023-12-29T02:05:16Z

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

+      requireBufferId = -1;
      status = StatusCode.NO_REGISTER;
      ShuffleServerMetrics.counterTotalRequireBufferFailed.inc();
+    } catch (NoBufferException e) {


How to handle NoBufferForHugePartitionException, I don't see these code/.

zuston

Could you add some tests for huge partition no buffer exception ?

lifeSo · 2023-12-30T20:09:07Z

Could you add some tests for huge partition no buffer exception ?

Ok, I added sendDataAndRequireBufferTest() test case in ShuffleServerGrpcTest. Mainly:

@zuston I added new test case.

zuston

I hope the memory message of being limited by huge partition should be shown in client side rather than only having one exception in server

zuston · 2024-01-02T02:52:59Z

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

+      requireBufferId = -1;
      status = StatusCode.NO_REGISTER;
      ShuffleServerMetrics.counterTotalRequireBufferFailed.inc();
+    } catch (NoBufferException | NoBufferForHugePartitionException e) {


This looks weird, why not introducing new stateCode of NoBufferForHugePartitionException

jerqi · 2024-01-02T05:58:51Z

common/src/main/java/org/apache/uniffle/common/rpc/StatusCode.java

-  TIMEOUT(7),
-  ACCESS_DENIED(8),
-  INVALID_REQUEST(9),
+  NO_BUFFER_FOR_HUGE_PARTITION(3),


You should consider the compatility.

NO_BUFFER_FOR_HUGE_PARTITION(10)

+1. should be 11

zuston · 2024-01-02T09:45:05Z

common/src/main/java/org/apache/uniffle/common/rpc/StatusCode.java

-  TIMEOUT(7),
-  ACCESS_DENIED(8),
-  INVALID_REQUEST(9),
+  NO_BUFFER_FOR_HUGE_PARTITION(3),


+1. should be 11

zuston · 2024-01-02T09:45:20Z

integration-test/common/src/test/java/org/apache/uniffle/test/IntegrationTestBase.java

  protected static final int JETTY_PORT_1 = 19998;
  protected static final int JETTY_PORT_2 = 20040;
  protected static final String COORDINATOR_QUORUM = LOCALHOST + ":" + COORDINATOR_PORT_1;
+  protected static final String SHUFFLE_SERVER_METRICS_URL =


why need this?

zuston · 2024-01-02T09:47:07Z

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

  public static final String LOCAL_DISK_PATH_LABEL_ALL = "ALL";
  private static final String TOTAL_REQUIRE_BUFFER_FAILED = "total_require_buffer_failed";
-  private static final String TOTAL_REQUIRE_BUFFER_FAILED_FOR_HUGE_PARTITION =
+  private static final String TOTAL_REQUIRE_BUFFER_FAILED_FOR_NO_REGISTER =


this is not related with this PR. Right?

zuston · 2024-01-02T09:48:04Z

internal-client/src/main/java/org/apache/uniffle/client/impl/grpc/ShuffleServerGrpcClient.java

    final int backOffBase = 2000;
-    while (rpcResponse.getStatus() == RssProtos.StatusCode.NO_BUFFER) {
+    while (rpcResponse.getStatus() == RssProtos.StatusCode.NO_BUFFER
+        || rpcResponse.getStatus() == RssProtos.StatusCode.NO_BUFFER_FOR_HUGE_PARTITION) {


Could you expose the concrete status code into log message?

ping @lifeSo

jerqi · 2024-01-03T08:30:07Z

Could you reply all the comments from zuston?

lifeSo · 2024-01-03T10:01:41Z

**zuston ** reviewed Dec 28, 2023
View reviewed changes

Could you reply all the comments from zuston?

@jerqi I checked and find one place miss modify, and I corrected it.
I thin all comments is reply and corrected.

jerqi · 2024-01-03T10:06:11Z

**zuston ** reviewed Dec 28, 2023
View reviewed changes

Could you reply all the comments from zuston?

@jerqi I checked and find one place miss modify, and I corrected it. I thin all comments is reply and corrected.

You should reply the comment instead of marking the comment resolved. The reply is the your solution. Maybe it will be very simple like fixed. Let reviewer mark the comment resolved.

Update CoordinatorGrpcClient.java, remove un-necessary code 添加require buffer测试样例，加强原有测试样例 add NO_BUFFER_FOR_HUGE_PARTITION status code. add require_buffer_failed_for_huge_partition exception unit test. revert status code fix log msg add status code revert

zuston · 2024-01-08T03:38:20Z

#1425 Merged. And close this.

lifeSo added 12 commits December 11, 2023 21:51

添加堆外内存监控的代码

7ab778f

format code

df92f55

format code

1759f9a

file rename

01872d0

rename

a0f5ea6

format

84df9a4

format

0a9a90d

format

7ca74cf

Merge branch 'apache:master' into master

47de654

add file for commit

a4ac621

test

c4f4761

*** for test now ***

0d68f5c

lifeSo changed the title ~~[1119][Improvement] Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER~~ [1119][Improvement] [For Test Now] Dec 23, 2023

lifeSo added 3 commits December 23, 2023 21:06

for test

01a1dc6

test

2281ffa

for test

c6c52a5

lifeSo marked this pull request as draft December 26, 2023 03:40

lifeSo changed the title ~~[1119][Improvement] [For Test Now]~~ [1119] improvement: Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER Dec 26, 2023

lifeSo added 7 commits December 26, 2023 19:44

format code

f973f36

revert

f1455e5

modify for interface

e794948

try optimize

ee3a998

format

75e2a82

modify test case

509cf08

lifeSo marked this pull request as ready for review December 26, 2023 13:48

lifeSo added 3 commits December 26, 2023 22:06

remove un-necessary code

9f80f29

format log info

9e50fac

Merge branch 'master' into feature_1119_new_2023_12_23

f51924c

lifeSo requested review from jerqi and zuston December 26, 2023 15:39

lifeSo changed the title ~~[1119] improvement: Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER~~ [#1119] improvement: Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER Dec 27, 2023

zuston reviewed Dec 28, 2023

View reviewed changes

zuston reviewed Dec 29, 2023

View reviewed changes

zuston reviewed Jan 2, 2024

View reviewed changes

lifeSo force-pushed the feature_1119_new_2023_12_23 branch from b30aedc to a94121c Compare January 2, 2024 03:30

jerqi reviewed Jan 2, 2024

View reviewed changes

lifeSo force-pushed the feature_1119_new_2023_12_23 branch from a54636f to 096820f Compare January 2, 2024 07:31

lifeSo marked this pull request as draft January 2, 2024 07:40

lifeSo force-pushed the feature_1119_new_2023_12_23 branch from 096820f to 37b897c Compare January 2, 2024 07:58

lifeSo marked this pull request as ready for review January 2, 2024 07:59

lifeSo force-pushed the feature_1119_new_2023_12_23 branch from 37b897c to 0079888 Compare January 2, 2024 08:16

zuston reviewed Jan 2, 2024

View reviewed changes

lifeSo requested review from jerqi and zuston January 3, 2024 08:26

lifeSo force-pushed the feature_1119_new_2023_12_23 branch from ad3106c to 0fd91b2 Compare January 3, 2024 09:35

lifeSo force-pushed the feature_1119_new_2023_12_23 branch from 54687ce to 94abd01 Compare January 7, 2024 10:13

lifeSo force-pushed the feature_1119_new_2023_12_23 branch from 94abd01 to 9085751 Compare January 7, 2024 10:15

lifeSo mentioned this pull request Jan 7, 2024

[#1119] improvement(client): Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION #1425

Merged

zuston closed this Jan 8, 2024

lifeSo deleted the feature_1119_new_2023_12_23 branch January 8, 2024 03:40

[#1119] improvement: Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER #1393

[#1119] improvement: Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER #1393

Uh oh!

Conversation

lifeSo commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

codecov-commenter commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuston left a comment

Choose a reason for hiding this comment

Uh oh!

lifeSo commented Dec 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerqi commented Jan 3, 2024

Uh oh!

lifeSo commented Jan 3, 2024

Uh oh!

jerqi commented Jan 3, 2024

Uh oh!

zuston commented Jan 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lifeSo commented Dec 23, 2023 •

edited

Loading

codecov-commenter commented Dec 23, 2023 •

edited

Loading

lifeSo commented Dec 30, 2023 •

edited

Loading