Repository navigation
[Dataflow Streaming] [MultiKey] Handle work cancellation exceptions in WorkFailureProcessor. - #40955
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR handles cyclic causal chains when inspecting exceptions in WorkCancellingException, WorkItemCancelledException, and WorkFailureProcessor, preventing StackOverflowErrors or infinite loops during failure processing. The implementation using identity-based Sets (Collections.newSetFromMap(new IdentityHashMap<>())) and the accompanying unit tests are clean, idiomatic, and thorough.
🔍 General Feedback
- The use of
IdentityHashMap-backed sets avoids invoking potentially expensive or state-dependentequals/hashCodemethods onThrowableinstances while accurately detecting reference cycles. - Loop termination conditions in
WorkCancellingException,WorkItemCancelledException, andWorkFailureProcessor.tryUnwrapExceptionare safe and preserve existing unwrapping semantics. - Unit tests in
WorkFailureProcessorTestcover both cyclic unwrapping and cyclic cause inspection for the cancellation exceptions.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
99df0c2 to
6c2679b
Compare
When processing a multi-key bundle, if one key fails due to a Windmill work cancellation (WorkCancellingException or WorkItemCancelledException), unaffected sibling keys in the batch must not be misclassified as uncaught user exceptions. This change adds WorkItemCancelledException.isWorkItemCancelledException(), updates WorkFailureProcessor.evaluateRetry() to return RetryEvaluation.RETRY_LOCALLY without reporting the failure or sleeping, and prevents infinite loops on circular causal chains in both isWorkCancellingException and isWorkItemCancelledException.
6c2679b to
25d1573
Compare
github-actions
Bot
added
size/m
|
R: @scwhittle |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| Set<Throwable> seen = Collections.newSetFromMap(new IdentityHashMap<>()); | ||
| @Nullable Throwable throwable = t; | ||
| while (throwable != null) { | ||
| while (throwable != null && seen.add(throwable)) { |
There was a problem hiding this comment.
do we actually have such cycles or is this more theoretical?
Is it worth avoiding the allocation in the case the input throwable doesn't have a cause?
I think there are other such cases where we iterate a throwable chain. Maybe we should have a helper taking the throwable class and doing this iteration, and use it in other cases too.
There was a problem hiding this comment.
do we actually have such cycles or is this more theoretical?
Theoretical. Updated to avoid allocations and extracted logic to a common util.
| } | ||
|
|
||
| if (WorkCancellingException.isWorkCancellingException(parsedException) | ||
| || WorkItemCancelledException.isWorkItemCancelledException(parsedException)) { |
There was a problem hiding this comment.
Should we have a helper that checks both? We won't have to iterate the throwable chain twice
Or should we change WorkCancellingException to WorkItemCancelledException after setting failed in the Work.java?
There was a problem hiding this comment.
Updated to a helper checking both. Want to keep WorkCancellingException and WorkItemCancelledException separate as code catching them do different things today.
4d1b2e0 to
b72fb40
Compare
b72fb40 to
e05a432
Compare
| } | ||
|
|
||
| /** Returns whether {@code throwable} or any of its causes matches the given {@code predicate}. */ | ||
| public static boolean hasAnyCause(@Nullable Throwable throwable, Predicate<Throwable> predicate) { |
There was a problem hiding this comment.
how about hasMatchingCause? hasAnyCause is confusing to me
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40955 +/- ##
============================================
- Coverage 79.83% 75.96% -3.87%
- Complexity 0 2291 +2291
============================================
Files 494 259 -235
Lines 83575 14421 -69154
Branches 0 1487 +1487
============================================
- Hits 66722 10955 -55767
+ Misses 16853 2775 -14078
- Partials 0 691 +691
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
When processing a multi-key bundle, if one key fails due to a Windmill work cancellation (WorkCancellingException or WorkItemCancelledException), unaffected sibling keys in the batch must not be misclassified as uncaught user exceptions.
This change adds WorkItemCancelledException.isWorkItemCancelledException(), updates WorkFailureProcessor.evaluateRetry() to return RetryEvaluation.RETRY_LOCALLY without reporting the failure or sleeping, and prevents infinite loops on circular causal chains in both isWorkCancellingException and isWorkItemCancelledException.