-
Notifications
You must be signed in to change notification settings - Fork 217
Compensation example for Workflows #1333
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Cassandra Coyle <[email protected]>
@cicoyle I was looking at this PR. The example doesn't seem to show that compensations are executed, right? Was this already implemented in dotnet? Do we have any reference of how it was implemented in dotnet? Did you used that implementation as inspiration for this PR? |
Signed-off-by: Cassandra Coyle <[email protected]>
This is a new pattern entirely. There are no examples in any other sdk or in the dapr docs. Its a simple example to illustrate how users can use the compensation workflow pattern. |
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
Signed-off-by: Cassandra Coyle <[email protected]>
- "Forcing Failure to trigger compensation for activity: io.dapr.examples.workflows.compensation.BookCarActivity" | ||
- "******** executing compensation logic ********" | ||
- "Activity failed: Task 'io.dapr.examples.workflows.compensation.BookCarActivity' (#2) failed with an unhandled exception: Failed to book car" | ||
- "Error during compensation: The orchestrator is blocked and waiting for new inputs. This Throwable should never be caught by user code." |
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.
Would it be better to catch the OrchestratorException separately. Because that is expected and is not an error during compensation
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.
good point, let me change that. It would be cleaner to not catch that exception since its part of the normal workflow runtime control flow. Ive switched to catching the TaskFailedException.
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.
Overall LGTM.
I think most of the files are missing a new line. If we could add that, the PR is good to go.
ctx.getLogger().info("Flight cancellation completed: {}", flightCancelResult); | ||
break; | ||
} | ||
} catch (Exception ex) { |
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.
Can we use retries in the Compensation activities? If so would catching the Exception break the retry mechanism?
Signed-off-by: artur-ciocanu <[email protected]>
Signed-off-by: artur-ciocanu <[email protected]>
Signed-off-by: artur-ciocanu <[email protected]>
Signed-off-by: artur-ciocanu <[email protected]>
Signed-off-by: artur-ciocanu <[email protected]>
Signed-off-by: artur-ciocanu <[email protected]>
Signed-off-by: artur-ciocanu <[email protected]>
Signed-off-by: artur-ciocanu <[email protected]>
Signed-off-by: artur-ciocanu <[email protected]>
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.
@cicoyle awesome job! I love the docs related to Mechanical Markdown.
My only concern is the comment from @javier-aliaga regarding retries. Either we mention this is the code comment or add some note, so developers are aware how handling the exception might affect the retry behavior.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1333 +/- ##
============================================
+ Coverage 76.91% 78.13% +1.22%
- Complexity 1592 1830 +238
============================================
Files 145 223 +78
Lines 4843 5663 +820
Branches 562 601 +39
============================================
+ Hits 3725 4425 +700
- Misses 821 917 +96
- Partials 297 321 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add a compensation workflow example