You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhaomin1423 (via GitHub)" <gi...@apache.org> on 2023/10/14 01:59:47 UTC

[PR] [SPARK-455315] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

zhaomin1423 opened a new pull request, #43371:
URL: https://github.com/apache/spark/pull/43371

   <!--
   Thanks for sending a pull request!  Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
     2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
     3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
     4. Be sure to keep the PR description updated to reflect all changes.
     5. Please write your PR title to summarize what this PR proposes.
     6. If possible, provide a concise example to reproduce the issue for a faster review.
     7. If you want to add a new configuration, please read the guideline first for naming configurations in
        'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
     8. If you want to add or modify an error type or message, please read the guideline first in
        'core/src/main/resources/error/README.md'.
   -->
   
   ### What changes were proposed in this pull request?
   <!--
   Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. 
   If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
     1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
     2. If you fix some SQL features, you can provide some references of other DBMSes.
     3. If there is design documentation, please add the link.
     4. If there is a discussion in the mailing list, please add the link.
   -->
   use java.lang.ref.Cleaner instead of finalize() for RemoteBlockPushResolver
   
   ### Why are the changes needed?
   <!--
   Please clarify why the changes are needed. For instance,
     1. If you propose a new API, clarify the use case for a new API.
     2. If you fix a bug, you can clarify why it is a bug.
   -->
   The finalize() method has been marked as deprecated since Java 9 and will be removed in the future, java.lang.ref.Cleaner is the more recommended solution.
   
   ### Does this PR introduce _any_ user-facing change?
   <!--
   Note that it means *any* user-facing change including all aspects such as the documentation fix.
   If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
   If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
   If no, write 'No'.
   -->
   No
   
   ### How was this patch tested?
   <!--
   If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
   If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
   If tests were not added, please describe why they were not added and/or why it was difficult to add.
   If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
   -->
   Pass GitHub Actions
   
   ### Was this patch authored or co-authored using generative AI tooling?
   <!--
   If generative AI tooling has been used in the process of authoring this patch, please include the
   phrase: 'Generated-by: ' followed by the name of the tool and its version.
   If no, write 'No'.
   Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
   -->
   No


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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360503697


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,34 +1871,23 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        dataFile.delete();

Review Comment:
   Check and log



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360941537


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,35 +1871,27 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",

Review Comment:
   Make deletion failed messages in this method a `INFO` at a minimum, deletion could have failed due to other concurrent deletions (this is called from finalize as well).



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,35 +1871,27 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",

Review Comment:
   Make deletion failed messages in this method a `INFO` at a minimum, deletion could have failed due to other concurrent deletions (this is called from finalize/cleaner as well).



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360165594


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -537,7 +540,9 @@ void closeAndDeleteOutdatedPartitions(
     partitions
       .forEach((partitionId, partitionInfo) -> {
         synchronized (partitionInfo) {
-          partitionInfo.closeAllFilesAndDeleteIfNeeded(true);
+          AppShufflePartitionInfo.closeAllFilesAndDeleteIfNeeded(true, partitionInfo.dataFile,

Review Comment:
   ```
   CLEANER.register(this, new ResourceCleaner(dataFile, dataChannel, indexFile,
           metaFile, appAttemptShuffleMergeId, reduceId));
   ```
   Maybe we can assign it to `Cleaner.Cleanable cleanable`, and then we can change this to call `partitionInfo.cleanable.clean();` there.
   
   And then, IIRC,  if `partitionInfo.cleanable.clean();` is called manually,` ResouceCleaner.run` will not be executed repeatedly.
   
   
   
   
   



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360568448


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1887,33 +1881,45 @@ static void closeAllFilesAndDeleteIfNeeded(
         if (dataChannel.isOpen()) {
           dataChannel.close();
         }
-        if (delete) {
-          dataFile.delete();
-        }
       } catch (IOException ioe) {
         logger.warn("Error closing data channel for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
       }
       try {
         metaFile.close();
-        if (delete) {
-          metaFile.delete();
-        }
       } catch (IOException ioe) {
         logger.warn("Error closing meta file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
         }
       try {
         indexFile.close();
-        if (delete) {
-          indexFile.delete();
-        }
       } catch (IOException ioe) {
         logger.warn("Error closing index file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
       }
     }
 
+    private void deleteAllFiles() {
+      try {
+        dataFile.delete();
+      } catch (Exception exception) {
+        logger.warn("Error deleting data file for {} reduceId {}",
+            appAttemptShuffleMergeId, reduceId);

Review Comment:
   Thanks, fix it.



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

Re: [PR] [SPARK-45534] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43371:
URL: https://github.com/apache/spark/pull/43371#issuecomment-1762504715

   cc @mridulm FYI


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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1361412044


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -94,8 +95,8 @@
  */
 public class RemoteBlockPushResolver implements MergedShuffleFileManager {
 
+  private static final Cleaner CLEANER = Cleaner.create();
   private static final Logger logger = LoggerFactory.getLogger(RemoteBlockPushResolver.class);
-

Review Comment:
   Why you remove this line? Please revert it.



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360562956


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,34 +1871,26 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",
+            appAttemptShuffleMergeId, reduceId);
         }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+      } catch (Exception exception) {

Review Comment:
   OK



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

Re: [PR] [SPARK-45534] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on PR #43371:
URL: https://github.com/apache/spark/pull/43371#issuecomment-1762485442

   Please help me review it. @LuciferYang 


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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on PR #43371:
URL: https://github.com/apache/spark/pull/43371#issuecomment-1764695641

   There are some unrelated fail, I push again without making any changes after rebase master.


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

Re: [PR] [SPARK-45534] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1359110742


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1933,39 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {
+
+      @Override
+      public void run() {
+        try {

Review Comment:
   +1 reuse `AppShufflePartitionInfo#closeAllFilesAndDeleteIfNeeded`.



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43371:
URL: https://github.com/apache/spark/pull/43371#issuecomment-1764359088

   The change is fine to me. Could you take another look?  @mridulm @beliefer Thanks ~


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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1361416337


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -94,8 +95,8 @@
  */
 public class RemoteBlockPushResolver implements MergedShuffleFileManager {
 
+  private static final Cleaner CLEANER = Cleaner.create();
   private static final Logger logger = LoggerFactory.getLogger(RemoteBlockPushResolver.class);
-

Review Comment:
   done，thanks



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -94,8 +95,8 @@
  */
 public class RemoteBlockPushResolver implements MergedShuffleFileManager {
 
+  private static final Cleaner CLEANER = Cleaner.create();
   private static final Logger logger = LoggerFactory.getLogger(RemoteBlockPushResolver.class);
-

Review Comment:
   done，thanks



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360953142


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,35 +1871,27 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
+        }
+      } catch (Exception exception) {
+        logger.warn("Error deleting data file for {} reduceId {}",
+          appAttemptShuffleMergeId, reduceId);
       }
       try {
-        metaFile.close();
-        if (delete) {
-          metaFile.delete();
-        }
+        metaFile.delete();
       } catch (IOException ioe) {
-        logger.warn("Error closing meta file for {} reduceId {}",
-            appAttemptShuffleMergeId, reduceId);
-        }
+        logger.warn("Error deleting meta file for {} reduceId {}",
+          appAttemptShuffleMergeId, reduceId);
+      }

Review Comment:
   org.apache.spark.network.shuffle.RemoteBlockPushResolver.MergeShuffleFile#delete throw IOException, if drop it in this method, the delete method need modify.



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360563141


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -827,9 +824,7 @@ public MergeStatuses finalizeShuffleMerge(FinalizeShuffleMerge msg) {
                 msg.appAttemptId, msg.shuffleId, msg.shuffleMergeId, partition.reduceId,
                 ioe.getMessage());
           } finally {
-            AppShufflePartitionInfo.closeAllFilesAndDeleteIfNeeded(false, partition.dataFile,
-                partition.getDataChannel(), partition.getIndexFile(), partition.getMetaFile(),
-                partition.getAppAttemptShuffleMergeId(), partition.getReduceId());
+            partition.cleanable.clean();

Review Comment:
   I see. Thank you!



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360966112


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,35 +1871,27 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
+        }
+      } catch (Exception exception) {
+        logger.warn("Error deleting data file for {} reduceId {}",
+          appAttemptShuffleMergeId, reduceId);
       }
       try {
-        metaFile.close();
-        if (delete) {
-          metaFile.delete();
-        }
+        metaFile.delete();
       } catch (IOException ioe) {
-        logger.warn("Error closing meta file for {} reduceId {}",
-            appAttemptShuffleMergeId, reduceId);
-        }
+        logger.warn("Error deleting meta file for {} reduceId {}",
+          appAttemptShuffleMergeId, reduceId);
+      }

Review Comment:
   Thanks, drop them



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360130919


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,7 +1875,14 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    static void closeAllFilesAndDeleteIfNeeded(

Review Comment:
   This method can be made `private`



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,7 +1875,14 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    static void closeAllFilesAndDeleteIfNeeded(

Review Comment:
   This method can be made `private` ?



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360148902


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1946,29 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    AppAttemptShuffleMergeId getAppAttemptShuffleMergeId() {
+      return appAttemptShuffleMergeId;
+    }
+
+    int getReduceId() {
+      return reduceId;
+    }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {

Review Comment:
   @mridulm Thanks for the explanation.



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360518425


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1887,33 +1881,45 @@ static void closeAllFilesAndDeleteIfNeeded(
         if (dataChannel.isOpen()) {
           dataChannel.close();
         }
-        if (delete) {
-          dataFile.delete();
-        }
       } catch (IOException ioe) {
         logger.warn("Error closing data channel for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
       }
       try {
         metaFile.close();
-        if (delete) {
-          metaFile.delete();
-        }
       } catch (IOException ioe) {
         logger.warn("Error closing meta file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
         }
       try {
         indexFile.close();
-        if (delete) {
-          indexFile.delete();
-        }
       } catch (IOException ioe) {
         logger.warn("Error closing index file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
       }
     }
 
+    private void deleteAllFiles() {
+      try {
+        dataFile.delete();
+      } catch (Exception exception) {
+        logger.warn("Error deleting data file for {} reduceId {}",
+            appAttemptShuffleMergeId, reduceId);

Review Comment:
   Indentation: 2 spaces



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -827,9 +824,7 @@ public MergeStatuses finalizeShuffleMerge(FinalizeShuffleMerge msg) {
                 msg.appAttemptId, msg.shuffleId, msg.shuffleMergeId, partition.reduceId,
                 ioe.getMessage());
           } finally {
-            AppShufflePartitionInfo.closeAllFilesAndDeleteIfNeeded(false, partition.dataFile,
-                partition.getDataChannel(), partition.getIndexFile(), partition.getMetaFile(),
-                partition.getAppAttemptShuffleMergeId(), partition.getReduceId());
+            partition.cleanable.clean();

Review Comment:
   `partitionInfo.deleteAllFiles();` ?



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360965339


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,35 +1871,27 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",

Review Comment:
   update it to ```INFO```



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360558718


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,34 +1871,26 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",
+            appAttemptShuffleMergeId, reduceId);
         }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+      } catch (Exception exception) {

Review Comment:
   It may be throw a SecurityException



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360945069


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,35 +1871,27 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
+        }
+      } catch (Exception exception) {
+        logger.warn("Error deleting data file for {} reduceId {}",
+          appAttemptShuffleMergeId, reduceId);

Review Comment:
   We dont need to handle security manager related exceptions, they are not relevant for spark - we can drop this catch block.



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360435276


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1734,6 +1735,8 @@ public static class AppShufflePartitionInfo {
     private int numIOExceptions = 0;
     private boolean indexMetaUpdateFailed;
 
+    private static final Cleaner CLEANER = Cleaner.create();

Review Comment:
   Done, thanks



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on PR #43371:
URL: https://github.com/apache/spark/pull/43371#issuecomment-1764897751

   > A few comments, mostly looks good.
   
   All has been modified. Thank you very much for your comments.


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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on PR #43371:
URL: https://github.com/apache/spark/pull/43371#issuecomment-1767747660

   Thank you for your review and providing valuable suggestions. @mridulm @LuciferYang @beliefer 
   
   


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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360481397


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1924,49 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {
+
+      @Override
+      public void run() {
+        closeAllFiles(dataChannel,
+            indexFile, metaFile, appAttemptShuffleMergeId, reduceId);
+      }
+
+      private void closeAllFiles(
+        FileChannel dataChannel,

Review Comment:
   Indentation: 4 spaces



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,34 +1871,23 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        dataFile.delete();

Review Comment:
   For `dataFile.delete()`, the return value should be checked. If it returns false, then the deletion has failed.
   
   



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1924,49 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {
+
+      @Override
+      public void run() {
+        closeAllFiles(dataChannel,
+            indexFile, metaFile, appAttemptShuffleMergeId, reduceId);

Review Comment:
   Indentation: 2 spaces



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360085452


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1946,29 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    AppAttemptShuffleMergeId getAppAttemptShuffleMergeId() {
+      return appAttemptShuffleMergeId;
+    }
+
+    int getReduceId() {
+      return reduceId;
+    }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {

Review Comment:
   I'm a bit confused.
   If so, `ResourceCleaner` hold the references of `FileChannel`, `MergeShuffleFile` and so on.



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360165594


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -537,7 +540,9 @@ void closeAndDeleteOutdatedPartitions(
     partitions
       .forEach((partitionId, partitionInfo) -> {
         synchronized (partitionInfo) {
-          partitionInfo.closeAllFilesAndDeleteIfNeeded(true);
+          AppShufflePartitionInfo.closeAllFilesAndDeleteIfNeeded(true, partitionInfo.dataFile,

Review Comment:
   ```
   CLEANER.register(this, new ResourceCleaner(dataFile, dataChannel, indexFile,
           metaFile, appAttemptShuffleMergeId, reduceId));
   ```
   Maybe we can assign it to `Cleaner.Cleanable cleanable`, and then we can change this to call `partitionInfo.cleanable.clean();` there.
   
   And then, IIRC,  if `partitionInfo.cleanable.clean();` is called manually,` ResouceCleaner.run` will not be executed repeatedly.
   
   
   
   
   



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -822,7 +827,9 @@ public MergeStatuses finalizeShuffleMerge(FinalizeShuffleMerge msg) {
                 msg.appAttemptId, msg.shuffleId, msg.shuffleMergeId, partition.reduceId,
                 ioe.getMessage());
           } finally {
-            partition.closeAllFilesAndDeleteIfNeeded(false);
+            AppShufflePartitionInfo.closeAllFilesAndDeleteIfNeeded(false, partition.dataFile,

Review Comment:
   ```
   CLEANER.register(this, new ResourceCleaner(dataFile, dataChannel, indexFile,
           metaFile, appAttemptShuffleMergeId, reduceId));
   ```
   Maybe we can assign it to `Cleaner.Cleanable cleanable`, and then we can change this to call `partitionInfo.cleanable.clean();` there.
   
   And then, IIRC,  if `partitionInfo.cleanable.clean();` is called manually,` ResouceCleaner.run` will not be executed repeatedly.
   
   
   
   
   



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360947933


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1927,54 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    @VisibleForTesting
+    Cleaner.Cleanable getCleanable() {
+      return cleanable;
+    }
+
+    private record ResourceCleaner(
+        File dataFile,

Review Comment:
   Drop `dataFile` ?



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43371:
URL: https://github.com/apache/spark/pull/43371#issuecomment-1767743867

   Merged to master.
   Thanks for working on this @zhaomin1423 !
   
   Thanks for the reviews @LuciferYang, @beliefer :-)


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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360436665


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -537,7 +540,9 @@ void closeAndDeleteOutdatedPartitions(
     partitions
       .forEach((partitionId, partitionInfo) -> {
         synchronized (partitionInfo) {
-          partitionInfo.closeAllFilesAndDeleteIfNeeded(true);
+          AppShufflePartitionInfo.closeAllFilesAndDeleteIfNeeded(true, partitionInfo.dataFile,

Review Comment:
   Thanks, modified based on your comments.



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360011335


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1946,29 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    AppAttemptShuffleMergeId getAppAttemptShuffleMergeId() {
+      return appAttemptShuffleMergeId;
+    }
+
+    int getReduceId() {
+      return reduceId;
+    }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {

Review Comment:
   Why not change the constructor accpets `AppShufflePartitionInfo`?



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360129540


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -481,7 +482,9 @@ void closeAndDeletePartitionsIfNeeded(
     appShuffleInfo.shuffles.forEach((shuffleId, shuffleInfo) -> shuffleInfo.shuffleMergePartitions
       .forEach((shuffleMergeId, partitionInfo) -> {
         synchronized (partitionInfo) {
-          partitionInfo.closeAllFilesAndDeleteIfNeeded(false);
+          AppShufflePartitionInfo.closeAllFilesAndDeleteIfNeeded(false, partitionInfo.dataFile,
+              partitionInfo.getDataChannel(), partitionInfo.getIndexFile(), partitionInfo.getMetaFile(),
+              partitionInfo.getAppAttemptShuffleMergeId(), partitionInfo.getReduceId());

Review Comment:
   You can keep `closeAllFilesAndDeleteIfNeeded`, and simply delegate to the new static method from there.
   This will also remove the need for `getReduceId` and `getAppAttemptShuffleMergeId`



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1946,29 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    AppAttemptShuffleMergeId getAppAttemptShuffleMergeId() {
+      return appAttemptShuffleMergeId;
+    }
+
+    int getReduceId() {
+      return reduceId;
+    }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {

Review Comment:
   `AppShufflePartitionInfo` is what goes out of scope - not the specific fields being referenced.



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1734,6 +1735,8 @@ public static class AppShufflePartitionInfo {
     private int numIOExceptions = 0;
     private boolean indexMetaUpdateFailed;
 
+    private static final Cleaner CLEANER = Cleaner.create();

Review Comment:
   nit: Move this static variable to the top of `RemoteBlockPushResolver` ?



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360964693


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1710,6 +1712,8 @@ public String toString() {
 
   /** Metadata tracked for an actively merged shuffle partition */
   public static class AppShufflePartitionInfo {
+
+    private static final Cleaner CLEANER = Cleaner.create();

Review Comment:
   Sorry, it was my mistake. It has been corrected.



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360165920


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1734,6 +1735,8 @@ public static class AppShufflePartitionInfo {
     private int numIOExceptions = 0;
     private boolean indexMetaUpdateFailed;
 
+    private static final Cleaner CLEANER = Cleaner.create();

Review Comment:
   +1



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360503923


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1924,49 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {
+
+      @Override
+      public void run() {
+        closeAllFiles(dataChannel,
+            indexFile, metaFile, appAttemptShuffleMergeId, reduceId);
+      }
+
+      private void closeAllFiles(
+        FileChannel dataChannel,

Review Comment:
   done



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1924,49 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {
+
+      @Override
+      public void run() {
+        closeAllFiles(dataChannel,
+            indexFile, metaFile, appAttemptShuffleMergeId, reduceId);

Review Comment:
   done



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360526040


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,34 +1871,26 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",
+            appAttemptShuffleMergeId, reduceId);
         }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+      } catch (Exception exception) {

Review Comment:
   hmm... In what scenarios would `dataFile.delete()` throw an exception?
   
   



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360573952


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1927,49 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {
+
+      @Override
+      public void run() {
+        closeAllFiles(dataChannel,
+          indexFile, metaFile, appAttemptShuffleMergeId, reduceId);

Review Comment:
   ```suggestion
           closeAllFiles(dataChannel, indexFile, metaFile, appAttemptShuffleMergeId, reduceId);
   ```



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360946397


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,35 +1871,27 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
+        }
+      } catch (Exception exception) {
+        logger.warn("Error deleting data file for {} reduceId {}",
+          appAttemptShuffleMergeId, reduceId);
       }
       try {
-        metaFile.close();
-        if (delete) {
-          metaFile.delete();
-        }
+        metaFile.delete();
       } catch (IOException ioe) {
-        logger.warn("Error closing meta file for {} reduceId {}",
-            appAttemptShuffleMergeId, reduceId);
-        }
+        logger.warn("Error deleting meta file for {} reduceId {}",
+          appAttemptShuffleMergeId, reduceId);
+      }

Review Comment:
   Drop all `IOException` catch blocks in this method - they are not thrown



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

Re: [PR] [SPARK-45534] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1359905254


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1756,6 +1759,8 @@ public static class AppShufflePartitionInfo {
       this.dataFilePos = 0;
       this.mapTracker = new RoaringBitmap();
       this.chunkTracker = new RoaringBitmap();
+      CLEANER.register(this, new ResourceCleaner(dataFile, dataChannel, indexFile,
+              metaFile, appAttemptShuffleMergeId, reduceId));

Review Comment:
   Can you submit a version for us to review together ：）



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1756,6 +1759,8 @@ public static class AppShufflePartitionInfo {
       this.dataFilePos = 0;
       this.mapTracker = new RoaringBitmap();
       this.chunkTracker = new RoaringBitmap();
+      CLEANER.register(this, new ResourceCleaner(dataFile, dataChannel, indexFile,
+              metaFile, appAttemptShuffleMergeId, reduceId));

Review Comment:
   Can you submit a version for us to review together ：）



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

Re: [PR] [SPARK-45534] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1359908418


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1756,6 +1759,8 @@ public static class AppShufflePartitionInfo {
       this.dataFilePos = 0;
       this.mapTracker = new RoaringBitmap();
       this.chunkTracker = new RoaringBitmap();
+      CLEANER.register(this, new ResourceCleaner(dataFile, dataChannel, indexFile,
+              metaFile, appAttemptShuffleMergeId, reduceId));

Review Comment:
   Updated, please help me review it.



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360016315


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1946,29 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    AppAttemptShuffleMergeId getAppAttemptShuffleMergeId() {
+      return appAttemptShuffleMergeId;
+    }
+
+    int getReduceId() {
+      return reduceId;
+    }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {

Review Comment:
   From api doc of java.lang.ref.Cleaner.
   ```
   The cleaning action is invoked only after the associated object becomes phantom reachable, so it is important that the object implementing the cleaning action does not hold references to the object.
   ```



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360524390


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1887,33 +1881,45 @@ static void closeAllFilesAndDeleteIfNeeded(
         if (dataChannel.isOpen()) {
           dataChannel.close();
         }
-        if (delete) {
-          dataFile.delete();
-        }
       } catch (IOException ioe) {
         logger.warn("Error closing data channel for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
       }
       try {
         metaFile.close();
-        if (delete) {
-          metaFile.delete();
-        }
       } catch (IOException ioe) {
         logger.warn("Error closing meta file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
         }
       try {
         indexFile.close();
-        if (delete) {
-          indexFile.delete();
-        }
       } catch (IOException ioe) {
         logger.warn("Error closing index file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
       }
     }
 
+    private void deleteAllFiles() {
+      try {
+        dataFile.delete();
+      } catch (Exception exception) {
+        logger.warn("Error deleting data file for {} reduceId {}",
+            appAttemptShuffleMergeId, reduceId);

Review Comment:
   +1



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360523564


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -827,9 +824,7 @@ public MergeStatuses finalizeShuffleMerge(FinalizeShuffleMerge msg) {
                 msg.appAttemptId, msg.shuffleId, msg.shuffleMergeId, partition.reduceId,
                 ioe.getMessage());
           } finally {
-            AppShufflePartitionInfo.closeAllFilesAndDeleteIfNeeded(false, partition.dataFile,
-                partition.getDataChannel(), partition.getIndexFile(), partition.getMetaFile(),
-                partition.getAppAttemptShuffleMergeId(), partition.getReduceId());
+            partition.cleanable.clean();

Review Comment:
   The original place where `closeAllFilesAndDeleteIfNeeded(false)` was, doesn't need to call `deleteAllFiles`.
   
   



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360941537


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,35 +1871,27 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",

Review Comment:
   Make deletion failed messages in this method a `INFO` at a minimum, deletion could have failed due to other concurrent deletions.



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360177836


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -537,7 +540,9 @@ void closeAndDeleteOutdatedPartitions(
     partitions
       .forEach((partitionId, partitionInfo) -> {
         synchronized (partitionInfo) {
-          partitionInfo.closeAllFilesAndDeleteIfNeeded(true);
+          AppShufflePartitionInfo.closeAllFilesAndDeleteIfNeeded(true, partitionInfo.dataFile,

Review Comment:
   Perhaps we could split `closeAllFilesAndDeleteIfNeeded` into two methods `closeAllFiles` and `deleteAllFiles`, then `closeAllFiles` can be moved into `ResourceCleaner.run()`. 
   
   This way, they don't have to be static. Here, we can change it to call 
   ```
   partitionInfo.cleanable.clean(); 
   partitionInfo.deleteAllFiles();
   ```
   and other places only need to call `partitionInfo.cleanable.clean()`.



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

Re: [PR] [SPARK-45534] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1359100137


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1933,39 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    private record ResourceCleaner(
+        File dataFile,
+        FileChannel dataChannel,
+        MergeShuffleFile indexFile,
+        MergeShuffleFile metaFile,
+        AppAttemptShuffleMergeId appAttemptShuffleMergeId,
+        int reduceId) implements Runnable {
+
+      @Override
+      public void run() {
+        try {

Review Comment:
   My only question is that this introduces some duplicate code with the `AppShufflePartitionInfo#closeAllFilesAndDeleteIfNeeded` method. Is there really no way to eliminate this duplication?
   
   



##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1756,6 +1759,8 @@ public static class AppShufflePartitionInfo {
       this.dataFilePos = 0;
       this.mapTracker = new RoaringBitmap();
       this.chunkTracker = new RoaringBitmap();
+      CLEANER.register(this, new ResourceCleaner(dataFile, dataChannel, indexFile,
+              metaFile, appAttemptShuffleMergeId, reduceId));

Review Comment:
   Indentation: 2 spaces



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360937228


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1710,6 +1712,8 @@ public String toString() {
 
   /** Metadata tracked for an actively merged shuffle partition */
   public static class AppShufflePartitionInfo {
+
+    private static final Cleaner CLEANER = Cleaner.create();

Review Comment:
    This is still within `AppShufflePartitionInfo`, see more [here](https://github.com/apache/spark/pull/43371#discussion_r1359142068).



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360150033


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -481,7 +482,9 @@ void closeAndDeletePartitionsIfNeeded(
     appShuffleInfo.shuffles.forEach((shuffleId, shuffleInfo) -> shuffleInfo.shuffleMergePartitions
       .forEach((shuffleMergeId, partitionInfo) -> {
         synchronized (partitionInfo) {
-          partitionInfo.closeAllFilesAndDeleteIfNeeded(false);
+          AppShufflePartitionInfo.closeAllFilesAndDeleteIfNeeded(false, partitionInfo.dataFile,
+              partitionInfo.getDataChannel(), partitionInfo.getIndexFile(), partitionInfo.getMetaFile(),
+              partitionInfo.getAppAttemptShuffleMergeId(), partitionInfo.getReduceId());

Review Comment:
   +1



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360968020


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1933,6 +1927,54 @@ public RoaringBitmap getMapTracker() {
     int getNumIOExceptions() {
       return numIOExceptions;
     }
+
+    @VisibleForTesting
+    Cleaner.Cleanable getCleanable() {
+      return cleanable;
+    }
+
+    private record ResourceCleaner(
+        File dataFile,

Review Comment:
   My mistake, drop it.



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

Re: [PR] [SPARK-45534] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1359905254


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1756,6 +1759,8 @@ public static class AppShufflePartitionInfo {
       this.dataFilePos = 0;
       this.mapTracker = new RoaringBitmap();
       this.chunkTracker = new RoaringBitmap();
+      CLEANER.register(this, new ResourceCleaner(dataFile, dataChannel, indexFile,
+              metaFile, appAttemptShuffleMergeId, reduceId));

Review Comment:
   Can you submit a revision for us to review together ：）
   
   
   



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

Re: [PR] [SPARK-45534] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1359901730


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1756,6 +1759,8 @@ public static class AppShufflePartitionInfo {
       this.dataFilePos = 0;
       this.mapTracker = new RoaringBitmap();
       this.chunkTracker = new RoaringBitmap();
+      CLEANER.register(this, new ResourceCleaner(dataFile, dataChannel, indexFile,
+              metaFile, appAttemptShuffleMergeId, reduceId));

Review Comment:
   change closeAllFilesAndDeleteIfNeeded to a static method, then use it.  is it ok?



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360959374


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,35 +1871,27 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
+        }
+      } catch (Exception exception) {
+        logger.warn("Error deleting data file for {} reduceId {}",
+          appAttemptShuffleMergeId, reduceId);
       }
       try {
-        metaFile.close();
-        if (delete) {
-          metaFile.delete();
-        }
+        metaFile.delete();
       } catch (IOException ioe) {
-        logger.warn("Error closing meta file for {} reduceId {}",
-            appAttemptShuffleMergeId, reduceId);
-        }
+        logger.warn("Error deleting meta file for {} reduceId {}",
+          appAttemptShuffleMergeId, reduceId);
+      }

Review Comment:
   We can drop that `IOException` - it was missed out earlier given we had `close` which can throw `IOException`



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "zhaomin1423 (via GitHub)" <gi...@apache.org>.
zhaomin1423 commented on code in PR #43371:
URL: https://github.com/apache/spark/pull/43371#discussion_r1360966656


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -1864,35 +1871,27 @@ private void finalizePartition() throws IOException {
       metaFile.getChannel().truncate(metaFile.getPos());
     }
 
-    void closeAllFilesAndDeleteIfNeeded(boolean delete) {
+    private void deleteAllFiles() {
       try {
-        if (dataChannel.isOpen()) {
-          dataChannel.close();
-        }
-        if (delete) {
-          dataFile.delete();
-        }
-      } catch (IOException ioe) {
-        logger.warn("Error closing data channel for {} reduceId {}",
+        if (!dataFile.delete()) {
+          logger.warn("Error deleting data file for {} reduceId {}",
             appAttemptShuffleMergeId, reduceId);
+        }
+      } catch (Exception exception) {
+        logger.warn("Error deleting data file for {} reduceId {}",
+          appAttemptShuffleMergeId, reduceId);

Review Comment:
   Got it, thanks



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

Re: [PR] [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver [spark]

Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #43371: [SPARK-45534][CORE] Use java.lang.ref.Cleaner instead of finalize for RemoteBlockPushResolver
URL: https://github.com/apache/spark/pull/43371


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