[#1086] [Doc] Simplify the Gluten code and add the doc #1322

summaryzb · 2023-11-17T14:24:54Z

What changes were proposed in this pull request?

Simplify the code when integrate with gluten
Add gluten uniffle doc

Why are the changes needed?

Integrate with gluten before gluten and uniffle release

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manual test since both projects are not released, after release, I'll add some tests in both projects

summaryzb · 2023-11-20T02:20:40Z

@leixm @yl09099 @jerqi
CommitID 0da1dd1164437a4cd in branch-0.8 seems like incomplete， it has make the unit test fail

summaryzb · 2023-11-20T02:33:10Z

Maybe we should import all the related commits in Dynamic shuffle server assignment , or just remove 0da1dd1164437a4cd

summaryzb · 2023-11-21T01:32:30Z

@advancedxy @xianjingfeng PTAL

jerqi · 2023-11-21T03:43:17Z

branch 0.8 has reverted 0da1dd1. Could you rebase this branch?

summaryzb · 2023-11-21T07:15:48Z

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

@@ -162,9 +162,10 @@ public WriteBufferManager(
  }

  /** add serialized columnar data directly when integrate with gluten */
-  public List<ShuffleBlockInfo> addPartitionData(int partitionId, byte[] serializedData) {
+  public List<ShuffleBlockInfo> addPartitionData(
+      int partitionId, byte[] serializedData, int serializedDataLength) {


Currently gluten reuse this serializedData byte array, serializedDataLength may be shorter than length of serializedData, so here change the signature of method addPartitionData

There's already another method

public List<ShuffleBlockInfo> addPartitionData(int partitionId, byte[] serializedData, int serializedDataLength, long start) {

I don't think we should change this signature. Caller could simply call the longer one, or we should add a new method:

public List<ShuffleBlockInfo> addPartitionData(int partitionId, byte[] serializedData, int serializedDataLength)

Ok, remove this and change the title

codecov-commenter · 2023-11-21T07:19:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cf25897) 53.86% compared to head (f4d9c14) 54.89%.

Additional details and impacted files

@@               Coverage Diff                @@
##             branch-0.8    #1322      +/-   ##
================================================
+ Coverage         53.86%   54.89%   +1.02%     
  Complexity         2601     2601              
================================================
  Files               391      371      -20     
  Lines             22445    20084    -2361     
  Branches           1879     1879              
================================================
- Hits              12090    11025    -1065     
+ Misses             9645     8420    -1225     
+ Partials            710      639      -71

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

advancedxy

This PR will be cherry-picked into master, right?

advancedxy · 2023-11-21T13:07:39Z

client-spark/spark3/src/main/java/org/apache/spark/shuffle/writer/RssShuffleWriter.java

@@ -86,7 +86,7 @@ public class RssShuffleWriter<K, V, C> extends ShuffleWriter<K, V> {
  private final Map<Integer, List<ShuffleServerInfo>> partitionToServers;
  private final Set<ShuffleServerInfo> shuffleServersForData;
  private final long[] partitionLengths;
-  private final boolean isMemoryShuffleEnabled;
+  protected final boolean isMemoryShuffleEnabled;


Rather than make this field protected, I would like to expose a method as follows:

protected boolean isMemoryShuffleEnabled() { return isMemoryShuffleEnabled; }

And access it anywhere.

Protected field and method can be accessed in the same scope, and the final field can not be changed except in the constructor.

advancedxy · 2023-11-21T13:12:07Z

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

@@ -162,9 +162,10 @@ public WriteBufferManager(
  }

  /** add serialized columnar data directly when integrate with gluten */
-  public List<ShuffleBlockInfo> addPartitionData(int partitionId, byte[] serializedData) {
+  public List<ShuffleBlockInfo> addPartitionData(
+      int partitionId, byte[] serializedData, int serializedDataLength) {


There's already another method

public List<ShuffleBlockInfo> addPartitionData(int partitionId, byte[] serializedData, int serializedDataLength, long start) {

I don't think we should change this signature. Caller could simply call the longer one, or we should add a new method:

public List<ShuffleBlockInfo> addPartitionData(int partitionId, byte[] serializedData, int serializedDataLength)

advancedxy · 2023-11-21T13:12:58Z

client-spark/common/src/test/java/org/apache/spark/shuffle/writer/WriteBufferManagerTest.java

    assertEquals(1, spyManager.getBuffers().size());
    assertEquals(0, shuffleBlockInfos.size());
-    shuffleBlockInfos = spyManager.addPartitionData(0, new byte[64]);
+    shuffleBlockInfos = spyManager.addPartitionData(0, new byte[64], 64);


could you also add a case when serializedDataLength is less than byteArray.size.

xianjingfeng · 2023-11-22T03:50:45Z

I am publishing 0.8.0-rc3. Is this pr necessary. @summaryzb

summaryzb · 2023-11-22T07:58:23Z

I am publishing 0.8.0-rc3. Is this pr necessary. @summaryzb

@xianjingfeng Yes, thanks

summaryzb · 2023-11-22T09:30:08Z

partitionId, byte[] serializedData, int serializedDataLength) {

summaryzb · 2023-11-22T09:31:35Z

This PR will be cherry-picked into master, right?
Yes

summaryzb · 2023-11-22T13:30:49Z

@advancedxy PTAL

xianjingfeng

LGTM

xianjingfeng · 2023-12-13T08:21:54Z

@summaryzb Could you cherry-pick this pr into master?

gluten integrate for branch-0.8

526d8e8

summaryzb force-pushed the branch-0.8 branch from 3b9d145 to 526d8e8 Compare November 21, 2023 07:08

summaryzb commented Nov 21, 2023

View reviewed changes

spotless check

d4751b0

summaryzb force-pushed the branch-0.8 branch from 2b01113 to d4751b0 Compare November 21, 2023 10:58

summaryzb changed the title ~~[#940] [FollowUp] Simplify the code when integrate with gluten~~ [#940] [FollowUp] Gluten reuse partition byte array Nov 21, 2023

advancedxy reviewed Nov 21, 2023

View reviewed changes

add WriteBufferManagerTest test

8bd89b3

summaryzb closed this Nov 22, 2023

summaryzb reopened this Nov 22, 2023

summaryzb added 2 commits November 22, 2023 17:50

todo

5165170

remove addPartition method, add some docs

f4d9c14

summaryzb changed the title ~~[#940] [FollowUp] Gluten reuse partition byte array~~ [#1086] [Doc] Add gluten uniffle doc Nov 22, 2023

xianjingfeng requested a review from advancedxy November 25, 2023 10:26

xianjingfeng approved these changes Nov 25, 2023

View reviewed changes

advancedxy approved these changes Nov 25, 2023

View reviewed changes

jerqi changed the title ~~[#1086] [Doc] Add gluten uniffle doc~~ [#1086] [Doc] Simplify the Gluten code and add the doc Nov 25, 2023

xianjingfeng merged commit aa25cfa into apache:branch-0.8 Nov 27, 2023
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1086] [Doc] Simplify the Gluten code and add the doc #1322

[#1086] [Doc] Simplify the Gluten code and add the doc #1322

summaryzb commented Nov 17, 2023 •

edited

Loading

summaryzb commented Nov 20, 2023

summaryzb commented Nov 20, 2023

summaryzb commented Nov 21, 2023

jerqi commented Nov 21, 2023

summaryzb Nov 21, 2023 •

edited

Loading

advancedxy Nov 21, 2023

summaryzb Nov 22, 2023

codecov-commenter commented Nov 21, 2023 •

edited

Loading

advancedxy left a comment

advancedxy Nov 21, 2023

summaryzb Nov 22, 2023

advancedxy Nov 21, 2023

advancedxy Nov 21, 2023

summaryzb Nov 22, 2023

xianjingfeng commented Nov 22, 2023 •

edited

Loading

summaryzb commented Nov 22, 2023

summaryzb commented Nov 22, 2023

summaryzb commented Nov 22, 2023

summaryzb commented Nov 22, 2023

xianjingfeng left a comment

xianjingfeng commented Dec 13, 2023

[#1086] [Doc] Simplify the Gluten code and add the doc #1322

[#1086] [Doc] Simplify the Gluten code and add the doc #1322

Conversation

summaryzb commented Nov 17, 2023 • 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?

summaryzb commented Nov 20, 2023

summaryzb commented Nov 20, 2023

summaryzb commented Nov 21, 2023

jerqi commented Nov 21, 2023

summaryzb Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

advancedxy Nov 21, 2023

Choose a reason for hiding this comment

summaryzb Nov 22, 2023

Choose a reason for hiding this comment

codecov-commenter commented Nov 21, 2023 • edited Loading

Codecov Report

advancedxy left a comment

Choose a reason for hiding this comment

advancedxy Nov 21, 2023

Choose a reason for hiding this comment

summaryzb Nov 22, 2023

Choose a reason for hiding this comment

advancedxy Nov 21, 2023

Choose a reason for hiding this comment

advancedxy Nov 21, 2023

Choose a reason for hiding this comment

summaryzb Nov 22, 2023

Choose a reason for hiding this comment

xianjingfeng commented Nov 22, 2023 • edited Loading

summaryzb commented Nov 22, 2023

summaryzb commented Nov 22, 2023

summaryzb commented Nov 22, 2023

summaryzb commented Nov 22, 2023

xianjingfeng left a comment

Choose a reason for hiding this comment

xianjingfeng commented Dec 13, 2023

summaryzb commented Nov 17, 2023 •

edited

Loading

summaryzb Nov 21, 2023 •

edited

Loading

codecov-commenter commented Nov 21, 2023 •

edited

Loading

xianjingfeng commented Nov 22, 2023 •

edited

Loading