-
Notifications
You must be signed in to change notification settings - Fork 16
Fix exception type when use RetriableTask in Fan in/out pattern #174
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
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.
LGTM so long as the suggested comment edit is incorporated.
| } catch (OrchestratorBlockedException | ContinueAsNewInterruption exception) { | ||
| throw exception; |
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.
just to confirm: can processNextEvent actually lead to a ContinueAsNewInterruption? My understanding is that processNextEvent just iterates through the history, and I would expect that ContinueAsNewInterruption would only occur when the customer code is actually invoked.
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.
Yes, ContinueAsNewInterruption would only occur when the customer code is actually invoked. The SDK code will never cause a ContinueAsNewInterruption. But when we iterate the history, if the customer code is throwing ContinueAsNewInterruption, we will let it go at this place instead of catching it.
| // ignore | ||
| /** | ||
| * the idea is that for any exception thrown here it should be handled | ||
| * in the do-while loop, the same exception should also be obtained when calling | ||
| * {code#future.get()} in the do-while loop, and it will be handled correctly | ||
| * in the catch block in the do-while loop. | ||
| */ | ||
| } | ||
| // Any exception happen we return true so that we will enter to the do-while block for the last time. |
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.
some nits on phrasing:
| // ignore | |
| /** | |
| * the idea is that for any exception thrown here it should be handled | |
| * in the do-while loop, the same exception should also be obtained when calling | |
| * {code#future.get()} in the do-while loop, and it will be handled correctly | |
| * in the catch block in the do-while loop. | |
| */ | |
| } | |
| // Any exception happen we return true so that we will enter to the do-while block for the last time. | |
| /** | |
| * We ignore the exception. Any Durable Task exceptions thrown here can be obtained when calling | |
| * {code#future.get()} in the implementation of 'await'. We defer to that loop to handle the exception. | |
| */ | |
| } | |
| // On an exception, we return true to the do-while block one last time. |
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 update the
do-whileblock logics so that all exception will be handle inside the blockdurabletask-java/client/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java
Line 1264 in e938278
Pull request checklist
CHANGELOG.mdAdditional information
Additional PR information