Add Nexus SignalWorkflowOperation #1770
Conversation
internal/interceptor.go
Outdated
| // | ||
| // Exposed as: [go.temporal.io/sdk/interceptor.Interceptor] |
There was a problem hiding this comment.
Hmm... how did this happen? @Quinn-With-Two-Ns looks like this is the third time this got repeated.
There was a problem hiding this comment.
I originally aliased temporalnexus.SignalWorkflowInput to this type and added it to the comment but then undid that and aliased it to the internal type. The automated tools complained about the checks until I ran with -fix which I guess just added another line again.
There was a problem hiding this comment.
I just went ahead and manually removed all the duplicates
There was a problem hiding this comment.
@yuandrew FYI might be a bug in your alias tool
temporalnexus/operation.go
Outdated
| // NewSignalWorkflowOperation is a helper for creating a synchronous nexus.Operation to deliver a signal. | ||
| // | ||
| // NOTE: Experimental | ||
| func NewSignalWorkflowOperation(name string) nexus.Operation[SignalWorkflowInput, nexus.NoValue] { |
There was a problem hiding this comment.
| func NewSignalWorkflowOperation(name string) nexus.Operation[SignalWorkflowInput, nexus.NoValue] { | |
| func NewWorkflowSignalOperation[T any](name string, func(ctx context.Context, input T, options nexus.StartOpertionOptions) SignalWorkflowInput) nexus.Operation[T, nexus.NoValue] { |
There was a problem hiding this comment.
So why require users to pass in a function to extract the required parameters from T instead of just giving them an input type which contains all the required values (wf ID, run ID, signal name, and signal arg)?
There was a problem hiding this comment.
The caller doesn't care that this is a signal, that's the handler's call.
test/nexus_test.go
Outdated
| var out string | ||
| require.NoError(t, run.Get(ctx, &out)) | ||
| require.Equal(t, "nexus", out) | ||
|
|
There was a problem hiding this comment.
Test the request ID and links are properly attached.
temporalnexus/operation.go
Outdated
| @@ -94,6 +93,28 @@ func NewSyncOperation[I any, O any]( | |||
| } | |||
| } | |||
|
|
|||
| // SignalWorkflowInput is the input to a NewSignalWorkflowOperation. | |||
| type SignalWorkflowInput = internal.ClientSignalWorkflowInput | |||
There was a problem hiding this comment.
Don't reuse the type from the interceptors, you want a type that contains the workflow ID, optional run ID and signal name.
|
Did we test what happens in a failure case like we signal a workflow that does not exist? I would think we would want that to fail the operation? |
There was a problem hiding this comment.
This is almost good to go AFAIC.
Something to consider here is that the behavior is different from the WorkflowRunOperation where the default is to pass through the operation input as workflow input. I'm wondering if users will find this inconsistency confusing.
@Quinn-With-Two-Ns you may want to comment on this.
| @@ -94,6 +93,38 @@ func NewSyncOperation[I any, O any]( | |||
| } | |||
| } | |||
|
|
|||
| // SignalWorkflowInput is the input to a NewSignalWorkflowOperation. | |||
There was a problem hiding this comment.
Please fix the docstring. It's not the input to the operation.
| WorkflowID string | ||
| RunID string | ||
| SignalName string | ||
| Arg any |
There was a problem hiding this comment.
Please document all of the public API fields.
| // NewSignalWorkflowOperation is a helper for creating a synchronous nexus.Operation to deliver a signal. | ||
| // | ||
| // NOTE: Experimental | ||
| func NewSignalWorkflowOperation[T any]( |
There was a problem hiding this comment.
| func NewSignalWorkflowOperation[T any]( | |
| func NewWorkflowSignalOperation[T any]( |
| } | ||
|
|
||
| // NewSignalWorkflowOperation is a helper for creating a synchronous nexus.Operation to deliver a signal. | ||
| // |
There was a problem hiding this comment.
Also call out that this ensures idempotency via the request ID mechanism (to some degree - violated if the workflow spans multiple runs) and the bidi linking is also important to call out (for now it's only uni-directional but we'll fix that).
I think the difference here is signal needs to know the workflow ID, where as starting a workflow generates the workflow ID. I am not strongly apposed to adding both for signal, I just worry it would encourage a design where uses are passing their workflow IDs in their signal inputs |
What was changed
Added a new type of synchronous Nexus operation for sending signals.
Why?
Convenience and request ID + links handling.
Checklist
Closes
How was this tested:
New functional tests