[CELEBORN-2460] Reply SHUFFLE_EXPIRED for a released shuffle instead of an empty file group - #3845
Open
shlomitubul wants to merge 1 commit into
Open
Conversation
…of an empty file group Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
LifecycleManagernow tracks the Celeborn shuffle ids released byunregisterShuffleand replies anew
StatusCode.SHUFFLE_EXPIREDfor them, instead of falling through to theSHUFFLE_UNREGISTEREDreply that the client turns into an empty file group.
expiredShuffleIdsis populated inunregisterShuffle, next to the existingunregisterShuffleTime.put, so the id is recorded at least oneceleborn.client.shuffle.expired.checkIntervalbeforeremoveExpiredShuffleerases any state.handleGetReducerFileGroupchecks it before theregisteredShufflecheck, since a released id isby then absent from
registeredShuffle.unregisterAppShuffle, when Spark has unregistered the app shuffle and noreader can exist.
The existing
SHUFFLE_UNREGISTEREDreply and its empty file group are unchanged, so a map stagethat wrote no data still reads as empty. No client-side change is needed:
SHUFFLE_EXPIREDfallsthrough the existing
defaultcase inloadFileGroupInternal, which returns a null file group andmakes
updateFileGroupthrowCelebornIOException.Why are the changes needed?
SHUFFLE_UNREGISTEREDcurrently means two different things and the LifecycleManager cannot tellthem apart once a shuffle has been swept:
registration is lazy, on the first push, so an empty file group is correct;
recomputed.
The second case is reachable while reduce tasks are still running. A task resolves a valid shuffle
id at the top of
CelebornShuffleReader.read; a fetch failure elsewhere in the stage invalidatesthat id and Spark reruns the map stage under a new id; with
celeborn.client.spark.fetch.cleanFailedShuffleenabled (CELEBORN-1896)FailedShuffleCleanerreleases the old id and
removeExpiredShuffleerases it. Spark does not cancel the tasks of afailed stage attempt (CELEBORN-1904), so a task from the first step can call
updateFileGroupafterwards, receive an empty file group, read zero records and report success.
Spark keeps the first successful result per partition even when it comes from a zombie stage
attempt, so that empty output becomes the partition's final output and the retry attempt skips it.
The application succeeds with missing rows and no failure appears in the logs. This was observed in
production on a large reduce stage after a single fetch failure.
Making the read path fail closed is complementary to CELEBORN-1904: cancelling the tasks of a failed
stage attempt narrows the window but cannot close it.
Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
A reduce task that reads a released shuffle now fails instead of silently returning no rows. The
failure is confined to tasks of an abandoned stage attempt, whose failures Spark does not count
(
TaskSetManagerskips accounting whenisZombie), and a live task that hit this would retry,resolve the current shuffle id and read the correct data. No configuration is added or changed.
How was this patch tested?
New case in
LifecycleManagerUnregisterShuffleSuiteasserting that, for the sameGetReducerFileGrouprequest, a registered shuffle repliesSUCCESS, the same id afterunregisterShuffleplus theremoveExpiredShufflesweep repliesSHUFFLE_EXPIRED, and an id thatwas never registered still replies
SHUFFLE_UNREGISTEREDwith an empty file group.LifecycleManagerUnregisterShuffleSuitepasses in full (7 tests) with-Pspark-3.5.