[CELEBORN-1498] Decide whether to reuse the shuffle id based on the appShuffle's numAvailableOutputs #2611
Conversation
This file contains 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
turboFei
reviewed
Jul 11, 2024
| @@ -20,6 +20,8 @@ | |||
| import java.util.HashSet; | |||
| import java.util.concurrent.ConcurrentHashMap; | |||
|
|
|||
| import org.apache.hadoop.classification.VisibleForTesting; | |||
There was a problem hiding this comment.
should be
import com.google.common.annotations.VisibleForTesting;
or just
// Visible for testing
There was a problem hiding this comment.
Fixed, thank you.
…ppShuffle's numAvailableOutputs
jiang13021
force-pushed
the
celeborn-1498
branch
from
2b898da to
31b8d34
Compare
|
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This issue was closed because it has been staled for 10 days with no activity. |
|
Once #2609 is merged, is this still relevant ? |
|
Thanks for the reminder @mridulm, seems not needed |
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?
Decide whether to reuse the shuffle id based on the appShuffle's numAvailableOutputs instead of deterministic level
Why are the changes needed?
In Spark, the DAGScheduler determines whether to reuse task output based on multiple factors, deterministic level is one of them, but not all. I constructed a case in the test, where a shuffleId should be regenerated instead of reused. IMO, celeborn should decide whether to reuse the shuffle id based on whether the map output is empty, which means to delegating the decision of reusing previous attempt results to the DAGScheduler.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Add a test: org.apache.celeborn.tests.spark.CelebornFetchFailureSuite#test("celeborn spark integration test - resubmit a barrier stage and do not reuse the shuffle id")