[CELEBORN-2434] Clamp endMapIndex in sliceSortedBufferByMapRange - #3815
Open
yew1eb wants to merge 1 commit into
Open
[CELEBORN-2434] Clamp endMapIndex in sliceSortedBufferByMapRange#3815yew1eb wants to merge 1 commit into
yew1eb wants to merge 1 commit into
Conversation
…full re-read of sorted memory-stored shuffle
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?
Add the missing
endMapIndex == Integer.MAX_VALUEclamp inShuffleBlockInfoUtils.sliceSortedBufferByMapRange, mirroring the existing guard in the sibling methodgetChunkOffsetsFromShuffleBlockInfos. Also add a unit test covering the full re-read (Integer.MAX_VALUE) case.Why are the changes needed?
When a memory-stored shuffle partition is read once with a map range (e.g. AQE skew join) and then re-read in full (
endIndex = Int.MaxValue, e.g. Spark ReusedExchange),FetchHandler.handleReduceOpenStreamInternalpasses the rawInt.MaxValueintogetSortedFileInfobecauseFileInfo.addStreamreturns false for the already-sorted file. The memory branch then loopsstartMapIndex..Integer.MAX_VALUEinsliceSortedBufferByMapRange— billions ofTreeMap.getcalls on the Netty fetch event loop, stalling all fetch/openStream traffic on that event loop for minutes to hours.The disk path is not affected (it goes through the already-clamped
getChunkOffsetsFromShuffleBlockInfos).Symptoms / evidence:
• MemoryStorageReusedExchangeSuite (a single tiny test with 1000 rows) takes 28–54 minutes instead of ~1 minute, both in CI and locally.
• Client side repeatedly logs BatchOpenStream for N cost 240006ms (exactly the 240s fetch timeout), followed by retries that stall again.
• Client logs Connection ... has been quiet for 4.0 m while there are outstanding requests against both workers' fetch ports.
• A thread dump taken while hung shows the fetch event loop spinning:
In production this means: memory storage + a range read followed by a full re-read of the same partition can wedge a worker's fetch event loop for an
unbounded time (the loop count scales with Int.MaxValue, multiplied by the number of files in the batch request).
Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
How was this patch tested?
testSliceSortedBufferByMapRangeWithMaxEndIndextoShuffleBlockInfoUtilsTest, asserting a full re-read withInteger.MAX_VALUEproduces exactly the same result as an explicit full-range read.ShuffleBlockInfoUtilsTest: all 6 tests pass.