Re: [PR] [#940] [FollowUp] Gluten reuse partition byte array [incubator-uniffle]

["advancedxy (via GitHub)" <gi...@apache.org>]

advancedxy commented on code in PR #1322:
URL: https://github.com/apache/incubator-uniffle/pull/1322#discussion_r1400574048


##########
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;

Review Comment:
   Rather than make this field `protected`, I would like to expose a method as follows:
   ```
   protected boolean isMemoryShuffleEnabled() {
     return isMemoryShuffleEnabled;
   }
   ```
   
   And access it anywhere.



##########
client-spark/common/src/test/java/org/apache/spark/shuffle/writer/WriteBufferManagerTest.java:
##########
@@ -211,10 +211,10 @@ public void addPartitionDataTest() {
     WriteBufferManager spyManager = spy(wbm);
     doReturn(512L).when(spyManager).acquireMemory(anyLong());
 
-    List<ShuffleBlockInfo> shuffleBlockInfos = spyManager.addPartitionData(0, new byte[64]);
+    List<ShuffleBlockInfo> shuffleBlockInfos = spyManager.addPartitionData(0, new byte[64], 64);
     assertEquals(1, spyManager.getBuffers().size());
     assertEquals(0, shuffleBlockInfos.size());
-    shuffleBlockInfos = spyManager.addPartitionData(0, new byte[64]);
+    shuffleBlockInfos = spyManager.addPartitionData(0, new byte[64], 64);

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



##########
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) {

Review Comment:
   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)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org