[GitHub] [spark] Ngone51 commented on a change in pull request #36000: [SPARK-38683][SHUFFLE] It is unnecessary to release the ShuffleManagedBufferIterator or ShuffleChunkManagedBufferIterator or ManagedBufferIterator buffers when the client channel's connection is terminated.

[GitBox <gi...@apache.org>]

Ngone51 commented on a change in pull request #36000:
URL: https://github.com/apache/spark/pull/36000#discussion_r838140176



##########
File path: common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java
##########
@@ -51,6 +51,7 @@
 
     // The channel associated to the stream
     final Channel associatedChannel;
+    final boolean shouldRelease;

Review comment:
       How about we rename it to "isBufferMaterializedOnNext" and add some comments to explain? I think it's more clear to tell why we don't release buffers if they're not materialized yet.

##########
File path: common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java
##########
@@ -59,10 +60,12 @@
     // Used to keep track of the number of chunks being transferred and not finished yet.
     final AtomicLong chunksBeingTransferred = new AtomicLong(0L);
 
-    StreamState(String appId, Iterator<ManagedBuffer> buffers, Channel channel) {
+    StreamState(String appId, Iterator<ManagedBuffer> buffers, Channel channel,
+        boolean shouldRelease) {

Review comment:
       ```suggestion
       StreamState(
           String appId,
           Iterator<ManagedBuffer> buffers,
           Channel channel,
           boolean shouldRelease) {
   ```

##########
File path: common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java
##########
@@ -215,12 +218,17 @@ public long chunksBeingTransferred() {
    * to be the only reader of the stream. Once the connection is closed, the stream will never
    * be used again, enabling cleanup by `connectionTerminated`.
    */
-  public long registerStream(String appId, Iterator<ManagedBuffer> buffers, Channel channel) {
+  public long registerStream(String appId, Iterator<ManagedBuffer> buffers, Channel channel,
+      boolean shouldRelease) {

Review comment:
       ```suggestion
     public long registerStream(
         String appId,
         Iterator<ManagedBuffer> buffers,
         Channel channel,
         boolean shouldRelease) {
   ```




-- 
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: reviews-unsubscribe@spark.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org