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: [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]
