Upgrade to Nexus SDK 0.2.0 #1802
Conversation
bergundy
force-pushed
the
nexus-0.2.0
branch
2 times, most recently
from
1524297 to
1a3b09b
Compare
temporalnexus/operation.go
Outdated
|
|
||
| // workflowRunOperationToken is the decoded form of the workflow run operation token. | ||
| type workflowRunOperationToken struct { | ||
| NamespaceName string `json:"ns"` |
There was a problem hiding this comment.
Is this format expected to be consistent across SDKs?
There was a problem hiding this comment.
I'd expect it to. Happy for others to chime in.
| @@ -2504,7 +2504,7 @@ func (env *testWorkflowEnvironmentImpl) ExecuteNexusOperation( | |||
| }, true) | |||
| case *nexuspb.StartOperationResponse_AsyncSuccess: | |||
| env.postCallback(func() { | |||
| opID = v.AsyncSuccess.GetOperationId() | |||
| opID = v.AsyncSuccess.GetOperationToken() | |||
There was a problem hiding this comment.
nit
| opID = v.AsyncSuccess.GetOperationToken() | |
| opToken = v.AsyncSuccess.GetOperationToken() |
| @@ -3241,7 +3241,7 @@ func (h *testNexusOperationHandle) startedCallback(opID string, e error) { | |||
| // Ignore duplciate starts. | |||
| return | |||
| } | |||
| h.operationID = opID | |||
| h.operationToken = opID | |||
There was a problem hiding this comment.
nit
| h.operationToken = opID | |
| h.operationToken = opToken |
| @@ -444,9 +467,17 @@ func (h *nexusTaskHandler) nexusHandlerErrorToProto(handlerErr *nexus.HandlerErr | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| var retryBehavior enumspb.NexusHandlerErrorRetryBehavior | |||
There was a problem hiding this comment.
| // Not using errors.As to be consistent ApplicationError checking with the rest of the SDK. | ||
| if appErr, ok := err.(*ApplicationError); ok && appErr.NonRetryable() { | ||
| return nexus.NewFailedOperationError(appErr) | ||
| return &nexus.HandlerError{ | ||
| // TODO(bergundy): Change this to a non retryable internal error after the 1.27.0 server release. |
There was a problem hiding this comment.
Can we create to track this? Also doesn't this need to be done before GA since it is a breaking change?
| @@ -444,9 +467,17 @@ func (h *nexusTaskHandler) nexusHandlerErrorToProto(handlerErr *nexus.HandlerErr | |||
| if err != nil { | |||
There was a problem hiding this comment.
Nit: it seems like we have all the Nexus error conversion logic here https://github.com/temporalio/sdk-go/pull/1802/files#diff-82a19cd3836c5a75e55d9c5d4400c0bfdd09029eb29fd1fa9755cf1a724f4654R86 except this one function. Not blocking but might be nice to group it.
There was a problem hiding this comment.
There's maybe a slight duplication but I don't think it's very critical apart from the grouping.
| return base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(data), nil | ||
| } | ||
|
|
||
| func loadWorkflowRunOperationToken(data string) (workflowRunOperationToken, error) { |
There was a problem hiding this comment.
During the meeting yesterday @cretz suggested we reserve v for a versioning field and fail if it is present. Did we decided not to do that?
There was a problem hiding this comment.
Ah thanks, that slipped my mind.
There was a problem hiding this comment.
Coming in the next commit.
| return base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(data), nil | ||
| } | ||
|
|
||
| func loadWorkflowRunOperationToken(data string) (workflowRunOperationToken, error) { |
There was a problem hiding this comment.
Ah thanks, that slipped my mind.
Not sure I understand what do you mean exactly by "need an interceptor" here? We don't have Temporal Nexus inbound or outbound interceptors that is on my plate. When we add them |
bergundy
force-pushed
the
nexus-0.2.0
branch
from
39fdda4 to
3372fb9
Compare
bergundy
force-pushed
the
nexus-0.2.0
branch
from
bacdf79 to
b14e6b0
Compare
Do not merge before we update the dependencies to use a published api-go and Nexus Go SDK.
Tested against server 1.26.2 and changes made in temporalio/temporal#7241.