Set Nexus operation ID on callback headers #1710
Conversation
There was a problem hiding this comment.
LGTM.
Another non-blocking request is to always have the server generate the links (SDK can generate as a fallback when used with an old server).
That way operation ID is always coming from the SDK and the link from the server.
We should also document the implications of changing the operation ID returned from WorkflowRunOperation.Start. Let's tell them to never do that because the ID is used for cancelation and is reporting completion from the server.
| @@ -324,12 +325,21 @@ func ExecuteUntypedWorkflow[R any]( | |||
| if startWorkflowOptions.TaskQueue == "" { | |||
| startWorkflowOptions.TaskQueue = nctx.TaskQueue | |||
| } | |||
| if startWorkflowOptions.ID == "" { | |||
There was a problem hiding this comment.
I'm wondering if we want to require setting workflow ID for Nexus operations. We require it in all Core based SDKs, maybe we can at least protect Nexus users. Not blocking this PR though. @cretz, @Quinn-With-Two-Ns to weigh in on this.
There was a problem hiding this comment.
I thought the Go SDK did require setting a workflow ID for Nexus operations? I think it makes sense to require it or at least default it to something deterministic so starting a workflow is idempotent
There was a problem hiding this comment.
Yes, this was my intent but I think the client will automatically fill in an ID unless one is provided. Need to verify this.
There was a problem hiding this comment.
We require it meaning I don't need the check for it being empty here? I looked for where a default would be set but didn't find anything that would set it before this point.
There was a problem hiding this comment.
I would change to ensure that the client doesn't auto generate an ID if one isn't set.
We should note this change in the release notes. I would consider it a backwards incompatible behavior change.
if startWorkflowOptions.ID == "" {
return nil, ErrMissingWorkflowID
}There was a problem hiding this comment.
@pdoerner this already changed for Java, please make sure to disallow empty workflow ID for nexus ops before merging this.
|
@bergundy re this comment
I didn't add any generation for links here; are you suggesting I should add the fallback? |
I'm saying we modify the server to return links and only generate them in the SDK in if the server doesn't return them in the StartWorkflowExecutionResponse. We'll need to consider when to return these links though. I opened an issue to track this, it's not blocking. |
internal/error.go
Outdated
| @@ -338,6 +338,9 @@ var ( | |||
|
|
|||
| // ErrSkipScheduleUpdate is used by a user if they want to skip updating a schedule. | |||
| ErrSkipScheduleUpdate = errors.New("skip schedule update") | |||
|
|
|||
| // ErrMissingWorkflowID is returned when trying to start an async Nexus operation but no workflow ID is set on the request. | |||
| ErrMissingWorkflowID = errors.New("workflow ID is unset") | |||
There was a problem hiding this comment.
nit can the error message make it clear that it was unset for a Nexus operation
temporalnexus/operation.go
Outdated
| if nexusOptions.CallbackHeader == nil { | ||
| nexusOptions.CallbackHeader = make(nexus.Header) | ||
| } | ||
| if _, set := nexusOptions.CallbackHeader[nexus.HeaderOperationID]; !set { |
There was a problem hiding this comment.
nit: should we use the case insensitive operations on the headers? I guess nexus.HeaderOperationID is now lower case so it isn't that important.
## What changed? <!-- Describe what has changed in this PR --> Added support for completing Nexus operations before the StartOperation response is received. Operation ID, StartTime, and StartLinks were added to the completion request so that a NexusOperationStarted event can be fabricated. Depends on nexus-rpc/sdk-go#26 and temporalio/sdk-go#1710 Bumps Temporal SDK version to `v1.30.1` Bumps Nexus SDK version to `v0.0.12` ## Why? <!-- Tell your future self why have you made these changes --> Better user experience. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> New functional test
* Set Nexus operation ID on callback headers * comment * return error on missing workflow ID * bump nexus sdk version * error message * tidy * tidy * tidy * tidy * tidy * case insensitive header * test * test
What was changed
Setting
nexus-operation-idheader on callback headers.Depends on nexus-rpc/sdk-go#26
Returns an error is the workflow ID is unset on the start options for a Nexus operation
Why?
This information is necessary for fabricating a NexusOperationStarted event if the completion is received before the response to a StartOperation request. This ID is set to the workflow ID unless the user provides a value for this header; user-provided values for this header are not overwritten.
Checklist
Closes
How was this tested:
Existing tests