[#2181]improvement(server): Replace ShuffleBlockInfo with ShufflePartitionedBlock in netty #2182

lwllvyb · 2024-10-15T06:12:31Z

What changes were proposed in this pull request?

On the server side of the netty mode, when the server receives the sendShuffleData message, it decodes it into an unnecessary ShuffleBlockInfo and then converts it to ShufflePartitionedBlock in handleSendShuffleDataRequest.

Why are the changes needed?

Fix: #2181

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Locally

lwllvyb · 2024-10-15T06:13:51Z

To ensure upgrade compatibility, only the server-side code was modified.

github-actions · 2024-10-15T06:40:44Z

Test Results

2 926 files +10 2 926 suites +10 6h 14m 27s ⏱️ + 5m 19s
1 088 tests ± 0 1 086 ✅ + 1 2 💤 ±0 0 ❌ ±0
13 630 runs +10 13 600 ✅ +11 30 💤 ±0 0 ❌ ±0

Results for commit 17d4112. ± Comparison against base commit 6e18b3e.

♻️ This comment has been updated with latest results.

jerqi · 2024-10-18T06:57:42Z

Is this a improvement?

lwllvyb · 2024-10-18T07:43:18Z

Is this a improvement?

In our stress test environment, it will generates 80million blocks. Removing useless ShuffleBlockInfo will reduce the JVM GC pressure.

jerqi · 2024-10-18T07:58:24Z

@maobaolong Could you help me review this?

maobaolong · 2024-10-18T07:59:17Z

@lwllvyb With pleasure!

maobaolong

@lwllvyb Thanks for this great work, I guess you could do the following changes for this PR.

Split the unrelated changes into another PR.
Create an issue to track the followup task and comment(TODO comment) to a place.
Calc the decodeLength by a simply and clear way rather than compute it everywhere. Maybe you can get it by ByteBuf position change.

maobaolong · 2024-10-18T08:01:59Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/Decoders.java

-    long crc = byteBuf.readLong();
-    long taskAttemptId = byteBuf.readLong();
+  public static Pair<Integer, ShufflePartitionedBlock> decodeShuffleBlockInfo(ByteBuf byteBuf) {
+    // partId Int


It would be better to create another issue to track the next plan base on this PR, and it could be clear and easy to track it by adding a issue link here as a TODO comment.

We should deprecate the SEND_SHUFFLE_DATA_REQUEST and removed in the future when release a new major version

When we upgrade to V2, we could send the encoded length to server, to avoid re-compute the encodedLength in the server side again.

maobaolong · 2024-10-18T08:31:54Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/Encoders.java

@@ -59,19 +59,27 @@ public static int encodeLengthOfShuffleServerInfo(ShuffleServerInfo shuffleServe
        + 2 * Integer.BYTES;
  }

+  public static int encodeLengthShuffleBlockInfoCommon() {


This could be a Constant?

maobaolong · 2024-10-18T09:25:41Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/Decoders.java

+    // freeMemory Long
+    byteBuf.skipBytes(8);
+
+    int shuffleBlockInfoLength =


You mean shuffleBlockInfoLength to shuffleBlockInfoEncodeLength?

maobaolong · 2024-10-18T09:32:11Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/SendShuffleDataRequest.java

+  }
+
+  public int getEncodedLength() {
+    return encodedLength;


This make me confusing, what is the different between getEncodedLength() and encodedLength?

maobaolong · 2024-10-18T09:35:47Z

server/src/main/java/org/apache/uniffle/server/netty/ShuffleServerNettyHandler.java

@@ -912,11 +905,9 @@ private ServerRpcAuditContext createAuditContext(
      auditLogger = AUDIT_LOGGER;
    }
    ServerRpcAuditContext auditContext = new ServerRpcAuditContext(auditLogger);
+    auditContext.withCreationTimeNs(System.nanoTime());


This could be another fix?

maobaolong · 2024-10-18T09:52:02Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/Decoders.java

+            + Encoders.encodeLengthOfShuffleServerInfos(serverInfos);
+
+    return Pair.of(
+        shuffleBlockInfoLength,


It could be better to calc the decodedLength by the byteBuf position

codecov-commenter · 2024-10-18T11:37:30Z

Codecov Report

Attention: Patch coverage is 62.06897% with 33 lines in your changes missing coverage. Please review.

Project coverage is 53.26%. Comparing base (78fe934) to head (1a6b65d).
Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
...ommon/netty/protocol/SendShuffleDataRequestV1.java	69.81%	16 Missing ⚠️
.../common/netty/protocol/SendShuffleDataRequest.java	0.00%	9 Missing ⚠️
...niffle/server/netty/ShuffleServerNettyHandler.java	0.00%	8 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2182      +/-   ##
============================================
+ Coverage     52.86%   53.26%   +0.39%     
+ Complexity     3385     3221     -164     
============================================
  Files           517      481      -36     
  Lines         28208    26073    -2135     
  Branches       2633     2467     -166     
============================================
- Hits          14913    13887    -1026     
+ Misses        12332    11281    -1051     
+ Partials        963      905      -58

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

lwllvyb · 2024-10-18T11:43:29Z

@maobaolong Fixed

maobaolong · 2024-10-20T02:15:07Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/Decoders.java

@@ -69,6 +70,32 @@ public static ShuffleBlockInfo decodeShuffleBlockInfo(ByteBuf byteBuf) {
        taskAttemptId);
  }

+  public static ShufflePartitionedBlock decodeShuffleBlockInfoV1(ByteBuf byteBuf) {


How about decodeShufflePartitionedBlockV1?

@lwllvyb Maybe you miss this comment

Thanks for your suggestion. I will change the function name.

@maobaolong Fixed.

maobaolong

Thanks for your update, it is better enough for code.

I comment inline to show the place that I concern, could you show that this change can not introduce memory leak?

maobaolong · 2024-10-20T05:33:16Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/SendShuffleDataRequestV1.java

+
+  @Override
+  public String getOperationType() {
+    return "sendShuffleData";


sendShuffleDataV1

Fixed it as you suggested.

maobaolong · 2024-10-20T05:52:14Z

server/src/main/java/org/apache/uniffle/server/netty/ShuffleServerNettyHandler.java

@@ -274,7 +273,7 @@ public void handleSendShuffleDataRequest(TransportClient client, SendShuffleData
          return;
        }
        final long start = System.currentTimeMillis();
-        shuffleBufferManager.releaseMemory(req.encodedLength(), false, true);
+        shuffleBufferManager.releaseMemory(req.getDecodedLength(), false, true);


Worried about req.getDecodedLength() is not equals to encodedLength since the encodedLength is calculated approximately, but req.getDecodedLength() is calculated accurately.

So It is necessary to have a test to ensure the releaseMemory is equals to requireMemory, so that there is not memory leak take place.

There already has a test class NettyProtocolTest to make sure that the encodedLength sent from client is same as the getDecodedLength of the server received.
Does we need a new test case?

@lwllvyb It is enough, thanks for show me this

maobaolong

@lwllvyb Thanks for this improvement, left a minor comment before, please take a look, otherwise, LGTM.

And look forward the next step base on this

lwllvyb · 2024-10-22T07:00:41Z

Ping @jerqi

…lock with netty mode.

jerqi · 2024-10-22T07:23:53Z

common/src/main/java/org/apache/uniffle/common/netty/protocol/SendShuffleDataRequestV1.java

+import org.apache.uniffle.common.ShufflePartitionedBlock;
+import org.apache.uniffle.common.util.ByteBufUtils;
+
+public class SendShuffleDataRequestV1 extends RequestMessage {


Why do we use v1 here?

The earliest version was a modification of the original class and there will be a lot of intrusive code and it will not be easy to review and understand.

SendShuffleDataRequestV1 class will not decode useless information to ShuffleBlockInfo which is temporary.

Will we have v2? If we don't have v2, maybe we should remove v1.

SendShuffleDataRequestV1 will be only used by Shuffle Server and SendShuffleDataRequest only be used by client. It is compatible with currently running cluster and serves as a transition.

Next stage, i will add SendShuffleDataRequestV2 which will be used by Shuffle Server and client. SendShuffleDataRequestV2 will not include useless information which is sent from client.

The request of the client is different from the one of the server. It's a little weird.

lwllvyb force-pushed the serverBlockInfoOpt01 branch from 7e3c3e3 to c67fa9f Compare October 15, 2024 06:16

lwllvyb closed this Oct 17, 2024

lwllvyb reopened this Oct 17, 2024

lwllvyb force-pushed the serverBlockInfoOpt01 branch from c67fa9f to f1745f3 Compare October 18, 2024 06:50

jerqi changed the title ~~fix[server] Replace ShuffleBlockInfo with ShufflePartitionedBlock in netty~~ [#2181]improvement(server): Replace ShuffleBlockInfo with ShufflePartitionedBlock in netty Oct 18, 2024

maobaolong suggested changes Oct 18, 2024

View reviewed changes

lwllvyb force-pushed the serverBlockInfoOpt01 branch 2 times, most recently from 7951a9e to 64af380 Compare October 18, 2024 12:02

lwllvyb closed this Oct 19, 2024

lwllvyb reopened this Oct 19, 2024

maobaolong reviewed Oct 20, 2024

View reviewed changes

lwllvyb force-pushed the serverBlockInfoOpt01 branch from 64af380 to ffb34d5 Compare October 21, 2024 07:33

lwllvyb closed this Oct 21, 2024

lwllvyb reopened this Oct 21, 2024

lwllvyb closed this Oct 21, 2024

lwllvyb reopened this Oct 21, 2024

maobaolong approved these changes Oct 22, 2024

View reviewed changes

lwllvyb force-pushed the serverBlockInfoOpt01 branch from ffb34d5 to c603565 Compare October 22, 2024 06:59

Improvement[server] Replace ShuffleBlockInfo with ShufflePartitionedB…

17d4112

…lock with netty mode.

lwllvyb force-pushed the serverBlockInfoOpt01 branch from c603565 to 17d4112 Compare October 23, 2024 09:55

jerqi reviewed Oct 23, 2024

View reviewed changes

[#2181]improvement(server): Replace ShuffleBlockInfo with ShufflePartitionedBlock in netty #2182

Are you sure you want to change the base?

[#2181]improvement(server): Replace ShuffleBlockInfo with ShufflePartitionedBlock in netty #2182

Conversation

lwllvyb commented Oct 15, 2024 • edited 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?

lwllvyb commented Oct 15, 2024

github-actions bot commented Oct 15, 2024 • edited Loading

Test Results

jerqi commented Oct 18, 2024

lwllvyb commented Oct 18, 2024

jerqi commented Oct 18, 2024

maobaolong commented Oct 18, 2024

maobaolong left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 18, 2024 • edited Loading

Codecov Report

lwllvyb commented Oct 18, 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

maobaolong left a 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

lwllvyb Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maobaolong left a comment

Choose a reason for hiding this comment

lwllvyb commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwllvyb Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwllvyb commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading

codecov-commenter commented Oct 18, 2024 •

edited

Loading

lwllvyb Oct 21, 2024 •

edited

Loading

lwllvyb Oct 23, 2024 •

edited

Loading