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

Add poststorage child workflows #1061

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Add poststorage child workflows #1061

merged 1 commit into from
Nov 13, 2024

Conversation

jraddaoui
Copy link
Collaborator

@jraddaoui jraddaoui commented Nov 7, 2024

Allow to configure a set of poststorage child workflows that will be
started after AIP storage. These workflows will receive the AIPUUID
as a parameter and the parent workflow will only wait for them to be
started by Temporal. They are started with a disconnected context and
using the abandon parent close policy, so they can continue running
after the parent workflow finishes, therefore their results are ignored.

Refs #886.

@jraddaoui jraddaoui self-assigned this Nov 7, 2024
@jraddaoui jraddaoui marked this pull request as draft November 7, 2024 07:16
@jraddaoui jraddaoui requested a review from djjuhasz November 7, 2024 17:52
Copy link
Collaborator

@djjuhasz djjuhasz left a comment

Choose a reason for hiding this comment

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

Thanks @jraddaoui. I had a couple of thoughts, but one doesn't require any changes. I think you need to address what happens if the poststorage workflow is called with an a3m vs. AM preservation system.

internal/workflow/processing.go Show resolved Hide resolved
internal/workflow/processing.go Show resolved Hide resolved
@jraddaoui jraddaoui requested a review from djjuhasz November 7, 2024 19:53
internal/workflow/processing.go Show resolved Hide resolved
@jraddaoui
Copy link
Collaborator Author

Yes, if a child workflow only works with AM and Enduro is configured to run with a3m, I'd prefer that child workflow to fail to know I misconfigured the system.

As I said, we may want to improve that at some point to be able to report the child workflows in the API/UI. For now they are just like webhooks to be able to trigger custom workflows after storage, and we only make sure those workflows are started by Temporal. Those workflows can be monitored elsewhere for now, until we decide if we want to add information about them in the API/UI.

I'll add some tests and ping you for a final review.

@jraddaoui jraddaoui force-pushed the dev/issue-886-poststorage branch from ae5f3d1 to 893c867 Compare November 13, 2024 02:32
@jraddaoui jraddaoui changed the title WIP: Add poststorage child workflows Add poststorage child workflows Nov 13, 2024
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 54.74%. Comparing base (92feb35) to head (a8191df).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
internal/workflow/processing.go 80.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1061      +/-   ##
==========================================
+ Coverage   54.55%   54.74%   +0.19%     
==========================================
  Files         103      104       +1     
  Lines        7512     7632     +120     
==========================================
+ Hits         4098     4178      +80     
- Misses       3164     3198      +34     
- Partials      250      256       +6     

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

@jraddaoui jraddaoui force-pushed the dev/issue-886-poststorage branch from 893c867 to 12ec6a6 Compare November 13, 2024 02:46
@jraddaoui jraddaoui marked this pull request as ready for review November 13, 2024 02:47
@jraddaoui jraddaoui requested a review from djjuhasz November 13, 2024 02:47
Copy link
Collaborator

@djjuhasz djjuhasz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Allow to configure a set of poststorage child workflows that will be
started after AIP storage. These workflows will receive the AIPUUID
as a parameter and the parent workflow will only wait for them to be
started by Temporal. They are started with a disconnected context and
using the abandon parent close policy, so they can continue running
after the parent workflow finishes, therefore their results are ignored.
@jraddaoui jraddaoui force-pushed the dev/issue-886-poststorage branch from 12ec6a6 to a8191df Compare November 13, 2024 19:34
@jraddaoui jraddaoui merged commit a8191df into main Nov 13, 2024
14 checks passed
@jraddaoui jraddaoui deleted the dev/issue-886-poststorage branch November 13, 2024 19:38
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.

2 participants