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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
