[GitHub] [spark] mridulm commented on a diff in pull request #35906: [SPARK-33236][shuffle] Enable Push-based shuffle service to store state in NM level DB for work preserving restart

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

mridulm commented on code in PR #35906:
URL: https://github.com/apache/spark/pull/35906#discussion_r886252782


##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -536,9 +645,20 @@ public MergeStatuses finalizeShuffleMerge(FinalizeShuffleMerge msg) {
         }
       }
       // Even when the mergePartitionsInfo is null, we mark the shuffle as finalized but the results
-      // sent to the driver will be empty. This cam happen when the service didn't receive any
+      // sent to the driver will be empty. This can happen when the service didn't receive any
       // blocks for the shuffle yet and the driver didn't wait for enough time to finalize the
       // shuffle.
+      if (db != null) {

Review Comment:
   There are 4 cases here:
   
   1. When `mergePartitionsInfo == null`.
      1. ESS never received a push for this mergeId, but we want to prevent future pushes, and so add a marker entry.
      2. In this case, makes sense to add the entry to level db as well.
   2. When `mergePartitionsInfo != null`, we have three cases:
      1. The first condition, in the `if`, results in exception - so that does not hit this case.
      2. The second is when `msg.shuffleMergeId > mergePartitionsInfo.shuffleMergeId`.
         1. We are scheduling a cleanup in this case - so all keys are going to get deleted.
      3. The happy path - we do want the entry to be added.
   
   On further thought, 2.2 is the issue above.
   Should we be doing `cleanUpAppShufflePartitionInfoInDB` in `closeAndDeletePartitionFiles` ?
   This is removing the finalization marker from the level db - which will continue to exist in the map.



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