Skip to content
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

worflows: activity retry policy #644

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

famarting
Copy link
Contributor

Description

first part of #541

also updated one of the existing examples to show how activity retry policies can be used

Issue reference

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@famarting famarting requested review from a team as code owners October 25, 2024 11:19
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 62.38%. Comparing base (6c59092) to head (a30f3a3).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
workflow/context.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #644      +/-   ##
==========================================
- Coverage   62.52%   62.38%   -0.14%     
==========================================
  Files          56       56              
  Lines        4139     4201      +62     
==========================================
+ Hits         2588     2621      +33     
- Misses       1425     1453      +28     
- Partials      126      127       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@cicoyle cicoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a solid implementation of retry policies for activities, just one potential signature refactor to accommodate the future addition of workflow retry policies. Also an ask - would it be possible to implement a simple activity that does continually fail - to test the retry logic?

workflow/activity_context.go Outdated Show resolved Hide resolved
workflow/activity_context.go Show resolved Hide resolved
Signed-off-by: Fabian Martinez <[email protected]>
Signed-off-by: Fabian Martinez <[email protected]>
@famarting
Copy link
Contributor Author

good to go

Signed-off-by: Fabian Martinez <[email protected]>
@famarting
Copy link
Contributor Author

can we merge?

Copy link
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a test for the activity to retry - I think it's crucial to validate this so we have some visibility if it breaks

Copy link
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding tests, just need to register the new activity

examples/workflow/main.go Outdated Show resolved Hide resolved
Signed-off-by: Fabian Martinez <[email protected]>
Copy link
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants