[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1726: [CELEBORN-804] ShuffleClient should cleanup shuffle infos when trigger unregisterShuffle.

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

pan3793 commented on code in PR #1726:
URL: https://github.com/apache/incubator-celeborn/pull/1726#discussion_r1267554409


##########
client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/SparkShuffleManager.java:
##########
@@ -154,13 +153,15 @@ public boolean unregisterShuffle(int shuffleId) {
     if (sortShuffleIds.contains(shuffleId)) {
       return sortShuffleManager().unregisterShuffle(shuffleId);
     }
-    if (appUniqueId == null) {
-      return true;
+    // For Spark driver side trigger unregister shuffle.
+    if (lifecycleManager != null) {
+      lifecycleManager.unregisterShuffle(shuffleId);
     }
-    if (shuffleClient == null) {
-      return false;
+    // For Spark executor side cleanup shuffle related info.

Review Comment:
   this is the key change, it's much simpler than the previous approach, I am wondering if we can revert the previous commit and re-submit this PR to make the change clearer.



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

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

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