-
Notifications
You must be signed in to change notification settings - Fork 107
Support signal_with_start #99
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.
Great addition, thank you! 🙌 I've suggested a few improvements inline. Can you please also add a spec to spec/unit/lib/temporal/grpc_client_spec.rb?
| @@ -0,0 +1,18 @@ | |||
| require 'activities/long_running_activity' | |||
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: Assuming we don't need this dependency
lib/temporal/client.rb
Outdated
| response.run_id | ||
| end | ||
|
|
||
| def compute_run_timeout(execution_options) |
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: This needs to be a private method
| 'signal_name', # the actual signal name | ||
| 'expected value', | ||
| 'signal_name', # the expected signal name, which the workflow takes as an arg | ||
| 0.1, # how long to sleep for |
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.
This looks a bit weird and definitely needs comments to distinguish different attributes. We should probably consider a few alternatives:
a. Keep the API similar to start_workflow and signal options to kwargs
Temporal.signal_with_start_workflow(
SignalWithStartWorkflow,
'signal_name',
0.1,
signal_name: 'signal name',
signal_input: 'expected value'
)b. Wrap signal attributes into a new object, which can also be assembled within the workflow's on_signal method
Temporal.signal_with_start_workflow(
SignalWithStartWorkflow,
Temporal::Signal.new('signal name', 'expected value'),
'signal_name',
0.1
)c. A combination of a & b (preserving start_workflow fingerprint)
Temporal.signal_with_start_workflow(
SignalWithStartWorkflow,
'signal_name',
0.1,
signal: Temporal::Signal.new('signal name', 'expected value')
)Open to other suggestions
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.
Another approach we might consider — instead of exposing another method, we can actually just adopt the start_workflow to accept a with_signal or signal option and then use a different API call based on the presence of that option
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.
Thanks! I was uncomfortable with this when I wrote it, but found no precedent for named params, decided not to break new stylistic ground, and produced confusion ;)
I went with named params, including one for workflow input because (I think) a single function signature can't have all three of *args, **kwargs, and named params. I can do away with workflow_input in favor of *args, but if the previous statement is correct, it would force signal_name/signal_input out of the signature. signal_name is required, and I don't like the idea of having a required **kwarg.
I didn't overload start_workflow because the java and golang sdks both use a separate method and I wanted to align with that. There's also a subtle difference between the semantics re: whether an existing workflow raises or not.
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.
Turns out you can have all 3, you just have to specify them in the correct order. Let me know what you think about workflow_input: vs *args. I still prefer it for the explicit juxtaposition with signal_input.
|
I'm going to bring this back with different factoring (the optional arguments to start_workflow) in a separate PR. |
…ner/dispatch-ordering Fix wait_until handler dispatch order
This PR adds support for signal_with_start_workflow_execution. It has some new tests that go with it:
And all existing units/integs pass too, except for
./spec/integration/converter_spec.rb, which times out (due to use of a task queue thatbin/workerdoesn't listen on) before and after this change.