Nexus error rehydration #1751
Nexus error rehydration #1751
Conversation
test/go.mod
Outdated
| @@ -67,6 +67,7 @@ require ( | |||
| ) | |||
|
|
|||
| replace ( | |||
| github.com/nexus-rpc/sdk-go => ../../nexus-sdk-go | |||
bergundy
force-pushed
the
nexus-error-rehydration
branch
from
186b1a6 to
8755496
Compare
| if !panic { | ||
| nctx.Log.Error("Context error while processing Nexus task", "error", ctx.Err()) | ||
| } | ||
| return nil, nil, errNexusTaskTimeout | ||
| } | ||
| if err != nil { | ||
| if !panic { | ||
| nctx.Log.Error("Handler returned error while processing Nexus task", "error", err) | ||
| } |
There was a problem hiding this comment.
| if !panic { | |
| nctx.Log.Error("Context error while processing Nexus task", "error", ctx.Err()) | |
| } | |
| return nil, nil, errNexusTaskTimeout | |
| } | |
| if err != nil { | |
| if !panic { | |
| nctx.Log.Error("Handler returned error while processing Nexus task", "error", err) | |
| } | |
| if !panic { | |
| nctx.Log.Error("Context error while processing Nexus task", tagError, ctx.Err()) | |
| } | |
| return nil, nil, errNexusTaskTimeout | |
| } | |
| if err != nil { | |
| if !panic { | |
| nctx.Log.Error("Handler returned error while processing Nexus task", tagError, err) | |
| } |
Or is it intentionally not using that tag?
| if !panic { | ||
| nctx.Log.Error("Context error while processing Nexus task", "error", ctx.Err()) | ||
| } |
There was a problem hiding this comment.
Arguably this should be logged regardless
There was a problem hiding this comment.
Yeah, I was debating but figured it'd be nicer to avoid the spam.
| if !panic { | ||
| nctx.Log.Error("Handler returned error while processing Nexus task", "error", err) | ||
| } |
There was a problem hiding this comment.
Arguably this log could just be after the actual call in its own if err != nil block, but not a big deal to have the bool
There was a problem hiding this comment.
err would not be nil here, it's filled in from the panic handler.
bergundy
force-pushed
the
nexus-error-rehydration
branch
from
8755496 to
085568e
Compare
| Failure: nexus.Failure{Message: "failed for test"}, | ||
| case "failed-plain-error": | ||
| return "", nexus.NewFailedOperationError(errors.New("failed for test")) | ||
| case "failed-app-error": |
There was a problem hiding this comment.
What are these cases for? They don't appear to be called by any test
There was a problem hiding this comment.
I left these and didn't commit the tests since we don't have server support yet. The tests are in a branch on my machine. I can start a draft PR that we'll merge once CLI with 1.26.2 is out.
What was changed
Add support for full Temporal error rehydration.
Any Temporal errors returned over the Nexus boundary will now automatically be fully rehydrated on the caller side.
Some notable points:
v0.1.0, which is a breaking change. Temporal Nexus users should refer to the release notes before upgrading the Temporal SDK.nexus.HandlerErrornow has aFailurerepresentation and is visible to the caller workflow.ApplicationErroras they do in the rest of the Temporal SDK.WorkflowExecutionError,QueryRejectedError, and non retryableApplicationErrors are no longer translated to bad request handler errors, instead they're translated toOperationErrors.Why?
Provide a more consistent experience for Temporal users and on par debugging experience with workflows and activities.
For reviewers
I've added the tests here to illustrate the E2E behavior but I'll have to wait for a server release that supports the new behavior before merging those tests. I may commit the tests in a separate PR just to get this code in or do a server RC for the full test coverage.