-
Notifications
You must be signed in to change notification settings - Fork 107
Add ParentClosePolicy #105
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
antstorm
left a comment
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.
Thank you for the PR. I've added a few comments, all should be easily addressable, but please let me know if you need any clarification.
Good call on getting the workflow_id when starting a workflow without explicitly providing it, we don't have a way to do that currently. We should probably return both a workflow_id and a run_id when scheduling a workflow, but that would be a breaking change
| end | ||
|
|
||
| def validate! | ||
| unless %i[terminate abandon request_cancel].include?(@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.
Can you please define these as constants so they can be used without hardcoding symbols?
| @namespace = options[:namespace] | ||
| @task_queue = options[:task_queue] || options[:task_list] | ||
| @retry_policy = options[:retry_policy] || {} | ||
| @parent_close_policy = options[:parent_close_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.
ExecutionOptions are shared between workflows and activities, so this wouldn't be the best place to add workflow-specific options. Can you please move these inline with the child workflow invocation?
| expect do | ||
| described_class.new(request_cancel_policy).to_proto | ||
| end.not_to raise_error | ||
| end |
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 you please split these into 3 examples? It's easier to figure out which ones have failed that way
I think it's possible to not to have break change? instead of changing the interface. I remember we have a |
|
@tim37021 yes, I was thinking that the most logical solution would be to return both workflow_id and run_id for a workflow_id, run_id = Temporal.start_workflow(MyWorkflow)However exposing this via metadata still works, but I'm not sure how you're planning to get the metadata. As for adding workflow_id to metadata — yes, that's something I've already implemented for cadence — coinbase/cadence-ruby#60, I'll replay that change over this repo today since others have asked about it as well |
|
There we go — #110 |
…oner/fix-tag Fix received_task tag
I found
ParentClosePolicyis not supported. Here's the PR.Story
I am implementing proxy workflow pattern mention HERE
I want to launch a child workflow and the main workflow will finish earlier then it's child. Due to default parent close policy. The child workflow will be killed as well.
Result
without
parent_close_policywith
parent_close_policy: :abandonUsage
By the way, how can I know my workflow id? Currently I explicitly names it.