Repository navigation
[Dataflow Streaming] [MultiKey] Handle work cancellation exceptions in WorkFailureProcessor. - #40955
arunpandianp wants to merge 1 commit into
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.
| } | ||
|
|
||
| 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?
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.