-
Notifications
You must be signed in to change notification settings - Fork 16
Fix the exception type for using retriable task in allOf method
#171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
allOf method
davidmrdavid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just leaving a question on the implementation strategy.
| /** | ||
| * Need to have two try-catch block, the first one we ignore the exception from the previous child task | ||
| * Once completed the future, method stack frame will pop out till `this.getChildTask().await();` | ||
| * in the first try-catch block. The child task now is previous child task we are awaiting on, for any exception | ||
| * throw by previous child task we should ignore, we only care about the last child task which is in next | ||
| * try-catch block. | ||
| */ | ||
| try { | ||
| // Always return the last child task result. | ||
| return this.getChildTask().await(); | ||
| } catch (Exception exception) { | ||
| /** | ||
| * If this RetriableTask is not configured as part of an allOf method (CompoundTask), | ||
| * it throws an exception, marking the orchestration as failed. However, if this RetriableTask | ||
| * is configured within an allOf method (CompoundTask), any exceptions are ignored. | ||
| * This approach ensures that when awaiting the future of the allOf method, | ||
| * it throws the CompositeTaskFailedException defined in its exceptionPath. | ||
| */ | ||
| if (!this.isInCompoundTask) throw exception; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm wondering if there might be a cleaner approach to implementing this. Ideally, I think the retriable task class should be unaware that it is inside a composite task. Otherwise, we're tightly coupling the logic.
Is it possible to instead try-catch the processNextEvent method here: https://github.com/microsoft/durabletask-java/blob/e938278aedbfadfd8b653bae9fe7afba30d05c18/client/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java#L1264C26-L1264C65
And ignore the TaskFailure exception that gets thrown during replay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's doable, let me test that out.
|
refer to #174 |
Issue describing the changes in this PR
Resolve #169
This PR fixes the exception type for using
RetriableTaskinallOfmethod.Description of the issue:
For
RetriableTaskinallOfmethod if the task failed for exception,CompositeTaskFailedExceptionis not thrown out instead it's aTaskFailedExceptionbeing thrown out. The reason is that theCompositeTaskFailedExceptionthat built in exceptionPath haven't get chance to be obtained at future.get(), ContextImplTask.this.processNextEvent() will first threw aTaskFailedExceptionFor example:
This PR ensures that any
RetriableTaskthat is inallOfmethod is marked asisInCompoundTaskso that later it uses this flag to decide whether to throw the exception or ignore it.Pull request checklist
CHANGELOG.mdAdditional information
Additional PR information