[CELEBORN-2430] Expose unexpected errors in client background threads - #3829
Open
Kalvin2077 wants to merge 2 commits into
Open
[CELEBORN-2430] Expose unexpected errors in client background threads#3829Kalvin2077 wants to merge 2 commits into
Kalvin2077 wants to merge 2 commits into
Conversation
Kalvin2077
changed the title
There was a problem hiding this comment.
Pull request overview
This PR aims to make failures in Celeborn client background tasks visible by logging unexpected Throwables and ensuring partition reader background failures are surfaced to callers via existing exception pathways.
Changes:
- Add logging for unexpected
Throwables inChangePartitionManagerandReleasePartitionManagerbackground tasks. - Broaden background error handling in
LocalPartitionReaderandDfsPartitionReaderto catchThrowableand propagate via their exception channels. - Add new unit tests covering background failure propagation (DFS/local readers) and verifying background task error logging (change/release partition managers).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client/src/main/java/org/apache/celeborn/client/read/DfsPartitionReader.java | Catch Throwable in fetch thread and propagate non-Exception via CelebornIOException (but currently risks hanging on interrupt). |
| client/src/main/java/org/apache/celeborn/client/read/LocalPartitionReader.java | Catch Throwable in local read task and wrap unexpected failures into CelebornIOException. |
| client/src/main/scala/org/apache/celeborn/client/ChangePartitionManager.scala | Add Throwable logging in both scheduler and per-shuffle background execution paths. |
| client/src/main/scala/org/apache/celeborn/client/ReleasePartitionManager.scala | Add Throwable logging for background release work to prevent silent failures. |
| client/src/test/java/org/apache/celeborn/client/ClientThreadErrorLoggingSuiteJ.java | New tests verifying unexpected background failures are logged for change/release partition managers. |
| client/src/test/java/org/apache/celeborn/client/read/PartitionReaderErrorHandlingSuiteJ.java | New tests verifying DFS/local partition readers propagate background failures through next(). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+116
to
+120
| case e: InterruptedException => | ||
| logError( | ||
| s"Batch handle change partition for shuffle $shuffleId interrupted.", | ||
| e) | ||
| throw e |
Contributor
Author
There was a problem hiding this comment.
Rethrowing InterruptedException already terminates the task, so interruption is not being suppressed. submit records the exceptional completion in its FutureTask; restoring the flag is needed when the exception is swallowed or converted, not when it is propagated.
Comment on lines
+287
to
+290
| } catch (InterruptedException e) { | ||
| // cancel a task for speculative, ignore this exception | ||
| logger.warn("Read thread is interrupted.", e); | ||
| } catch (Throwable t) { |
Contributor
Author
There was a problem hiding this comment.
Fixed. Set flag for CELEBORN-2154. Remove wrong comment.
Contributor
Author
|
@zaynt4606 @SteNicholas |
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?
Catch and log unexpected
Throwables in client background tasks. Partition readers also propagate background failures through their existing exception channel. No recovery behavior is added.Why are the changes needed?
ExecutorService.submitstores task failures in the returnedFuture. If thatFutureis not inspected, the failures can remain invisible. Catching onlyExceptionalso missesErrors.Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
How was this patch tested?
Added unit tests verifying that DFS and local partition readers propagate background errors, and that change- and release-partition tasks log them. The focused tests passed, including red/green verification for the manager tasks. Formatting checks also passed.