[CELEBORN-2183] Fix client tries to start a connection with excluded workers #3517
Conversation
SteNicholas
changed the title
There was a problem hiding this comment.
@r7raul1984, thanks for contribution. Please run dev/reformat to format.
ok |
SteNicholas
changed the title
SteNicholas
changed the title
SteNicholas
changed the title
client/src/main/java/org/apache/celeborn/client/read/CelebornInputStream.java
Outdated
Show resolved
Hide resolved
| fetchChunkRetryCnt++; | ||
| if (location.hasPeer() && !readSkewPartitionWithoutMapRange) { | ||
| if (location.hasPeer() | ||
| && !isExcluded(location.getPeer()) |
There was a problem hiding this comment.
It appears that isExcluded(location.getPeer()) now serves the same purpose as isExcluded(location) at line 455.
There was a problem hiding this comment.
My idea is to add this here to prevent throwing a new CelebornIOException at line 456.
|
@r7raul1984, any update? |
No more update |
|
@r7raul1984, the CI run failed. PTAL. |
ok |
how to rerun these checks? |
|
@r7raul1984, you'd better to close this pull request and reopen. |
It’s very strange — there are three checks that always fail randomly. |
|
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. |
3 participants
What changes were proposed in this pull request?
Do not attempt to fetch from any worker that has already been added to fetchExcludedWorkers. Instead, try fetching from a peer worker within the same PartitionLocation.
Why are the changes needed?
With the configuration celeborn.client.fetch.excludeWorkerOnFailure.enabled=true and celeborn.client.push.replicate.enabled=true,
when a fetch operation fails for a specific worker, that worker is added to fetchExcludedWorkers. However, subsequent attempts still try to fetch from this worker, causing repeated errors and slowing down shuffle fetch performance.
The logic should be modified so that any worker already in fetchExcludedWorkers is completely excluded from further fetch operations.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Manual test by debug