[SPARK-53898][CORE] Shuffle cleanup should not clean MapOutputTrackerMaster.shuffleStatuses in local cluster #52606
Conversation
| case _ => | ||
| } | ||
| // Shuffle cleanup should not clean up shuffle metadata on the driver | ||
| assert(mapOutputTrackerMaster.shuffleStatuses.nonEmpty) |
There was a problem hiding this comment.
Tests fails this assert before the fix.
| // through `ContextCleaner` when the shuffle is considered no longer referenced anywhere. | ||
| // Otherwise, we might hit exceptions if there is any subsequent access (which still | ||
| // reference that shuffle) to that shuffle metadata in `MapOutputTrackerMaster`. E.g., | ||
| // an ongoing subquery could access the same shuffle metadata which could have been |
There was a problem hiding this comment.
I am not sure if we should mention this, since the ideal behavior would be to terminate any subqueries when the main query completes.
There was a problem hiding this comment.
There could be a race even if we terminating the subquery?
There was a problem hiding this comment.
The main query has ended already. The subqueries results are anyway not going to be used (and is also a waste of resource in allowing it to continue).
There was a problem hiding this comment.
I understand we should cancel the running subquery and we should do it. My point is the running subquery could still access MapOutputTrackerMaster even if we cancel it right after the main query ends due to the race between them. So I think it's fine to mention it here.
| case RemoveShuffle(shuffleId) => | ||
| doAsync[Boolean](log"removing shuffle ${MDC(SHUFFLE_ID, shuffleId)}", context) { | ||
| if (mapOutputTracker != null) { | ||
| if (mapOutputTracker != null && !mapOutputTracker.isInstanceOf[MapOutputTrackerMaster]) { |
There was a problem hiding this comment.
Should this be in check be in unregistershuffle if we dont expect this to be called in master.
There can be someone else calling this same method on local mode in future?
There was a problem hiding this comment.
I was thinking about that. That way requires us to pass isLocal into MapOutputTrackerMaster, involves more changes. But I also agree it's safer.
There was a problem hiding this comment.
So with local mode, we can't clean up shuffle files only?
There was a problem hiding this comment.
I think shuffle cleanup still happens in local.
But the shuffle metadata cleanup only happens from ContextCleaner in driver.
|
cc: @cloud-fan |
| // an ongoing subquery could access the same shuffle metadata which could have been | ||
| // cleaned up after the main query completes. Note this currently only happens in local | ||
| // cluster where both driver and executor use the `MapOutputTrackerMaster`. | ||
| mapOutputTracker.unregisterShuffle(shuffleId) |
There was a problem hiding this comment.
so for non-local cluster, mapOutputTracker is at executors and unregisterShuffle only unregister the shuffle at executor side?
3 participants
What changes were proposed in this pull request?
This PR fixes a bug where
MapOutputTrackerMaster.shuffleStatusesis mistakenly cleaned up by Shuffle Cleanup feature in local cluster. The fix is done by avoid invokingmapOutputTracker.unregisterShuffle()inBlockManagerStorageEndpointwhenmapOutputTrackerisMapOutputTrackerMasteras it only happens in local cluster (non-local cluster should useMapOutputTrackerWorkerinstead).Why are the changes needed?
MapOutputTrackerMaster.shuffleStatusesshould only be cleaned whenContextCleanerconsiders the shuffle is no longer referenced anywhere. Otherwise, any subsequent access (which still reference that shuffle) to the same shuffle metadata inMapOutputTrackerMastercan lead toSparkExceptionand crash theSparkContext. Note this currently only happens in local cluster due to both driver and executor use theMapOutputTrackerMaster. E.g., an ongoing subquery could access the same shuffle metadata which could have been removed after the main query completes. See the detailed discussion at #52213 (comment).Does this PR introduce any user-facing change?
No.
How was this patch tested?
Updated the existing tests.
Was this patch authored or co-authored using generative AI tooling?
No.