-
Notifications
You must be signed in to change notification settings - Fork 5
feat(workflow): support workflow failure #57
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
feat(workflow): support workflow failure #57
Conversation
0e04abd to
bf5c38e
Compare
| ) | ||
| ) | ||
| ) | ||
| # TODO: handle cancellation error |
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.
| # TODO: handle cancellation error | |
| # TODO: handle cancellation error | |
| # TODO: handle unknown error, fail decision task and try again instead of breaking the engine |
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.
actually for other exceptions, they should be handled all together in the outter most layer WorkflowWorker.
| result = self._task.result() | ||
| try: | ||
| result = self._task.result() | ||
| except (CancelledError, InvalidStateError) as e: |
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.
Do we need to wrap cancelled error? There are different scenarios to iterate on this.
| if self._workflow_instance.is_done(): | ||
| if self._workflow_instance.is_done(): | ||
| try: | ||
| result = self._workflow_instance.get_result() |
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.
Nit: Maybe use the exception() method instead of try/catch to simplify the flow
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've thought about this. The exception() actually would raise CancelledError rather than just return it. So there isn't any simplification for this. I still want to keep the WorkflowInstance's API easier: one way to get the result or throw an exception.
Signed-off-by: Shijie Sheng <liouvetren@gmail.com>
Signed-off-by: Shijie Sheng <liouvetren@gmail.com>
Signed-off-by: Shijie Sheng <liouvetren@gmail.com>
8edb463 to
cad0b4c
Compare
| Decision( | ||
| fail_workflow_execution_decision_attributes=FailWorkflowExecutionDecisionAttributes( | ||
| failure=Failure( | ||
| reason=str(e), |
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.
reason needs to be class name of the underlying exception for retry policy
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.
fixed
| if self._workflow_instance.is_done(): | ||
| if self._workflow_instance.is_done(): | ||
| try: | ||
| result = self._workflow_instance.get_result() |
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've thought about this. The exception() actually would raise CancelledError rather than just return it. So there isn't any simplification for this. I still want to keep the WorkflowInstance's API easier: one way to get the result or throw an exception.
| ) | ||
| ) | ||
| ) | ||
| # TODO: handle cancellation error |
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.
actually for other exceptions, they should be handled all together in the outter most layer WorkflowWorker.
What changed?
Why?
add workflow failure handling.
How did you test it?
Integration test

Potential risks
Release notes
Documentation Changes