-
Notifications
You must be signed in to change notification settings - Fork 78
[DO NOT MERGE] Use Temporal Failures for Nexus APIs #627
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,7 @@ message ChildWorkflowExecutionFailureInfo { | |
| temporal.api.enums.v1.RetryState retry_state = 6; | ||
| } | ||
|
|
||
| // Representation of the Temporal SDK NexusOperationError object that is returned to workflow callers. | ||
| message NexusOperationFailureInfo { | ||
| // The NexusOperationScheduled event ID. | ||
| int64 scheduled_event_id = 1; | ||
|
|
@@ -91,6 +92,17 @@ message NexusHandlerFailureInfo { | |
| temporal.api.enums.v1.NexusHandlerErrorRetryBehavior retry_behavior = 2; | ||
| } | ||
|
|
||
| // Representation of the Nexus SDK OperationError object. | ||
| message NexusSDKOperationFailureInfo { | ||
| string state = 1; | ||
| } | ||
|
|
||
| // Representation of the Nexus SDK FailureError object. | ||
| message NexusSDKFailureErrorFailureInfo { | ||
| map<string, string> metadata = 1; | ||
| bytes data = 2; | ||
| } | ||
|
|
||
| message Failure { | ||
| string message = 1; | ||
| // The source this Failure originated in, e.g. TypeScriptSDK / JavaSDK | ||
|
|
@@ -125,6 +137,8 @@ message Failure { | |
| ChildWorkflowExecutionFailureInfo child_workflow_execution_failure_info = 12; | ||
| NexusOperationFailureInfo nexus_operation_execution_failure_info = 13; | ||
| NexusHandlerFailureInfo nexus_handler_failure_info = 14; | ||
| NexusSDKOperationFailureInfo nexus_sdk_operation_failure_info = 15; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am worried about adding this, can we confirm this error won't end up as the cause of a nexus operation failure in history?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the idea is that it would only be used in non-workflow callers and in handlers. But it may still show up in the cause chain in workflow, just not the top level failure.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When would this be part of the cause chain in a workflow?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since any failure can appear as a cause, there's nothing preventing users from setting
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that's true, my main concern is we don't change the current error chains type users see in workflows. Can we confirm that will remain the same?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, those will stay the same except for that we won't construct an implicit |
||
| NexusSDKFailureErrorFailureInfo nexus_sdk_failure_error_info = 16; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ option csharp_namespace = "Temporalio.Api.Nexus.V1"; | |
| import "google/protobuf/timestamp.proto"; | ||
| import "temporal/api/common/v1/message.proto"; | ||
| import "temporal/api/enums/v1/nexus.proto"; | ||
| import "temporal/api/failure/v1/message.proto"; | ||
|
|
||
| // A general purpose failure message. | ||
| // See: https://github.com/nexus-rpc/api/blob/main/SPEC.md#failure | ||
|
|
@@ -77,6 +78,12 @@ message CancelOperationRequest { | |
|
|
||
| // A Nexus request. | ||
| message Request { | ||
| message Capabilities { | ||
| // If set, handlers may use temporal.api.failure.v1.Failure instances to return failures to the server. | ||
| // This also allows handler and operation errors to have their own messages and stack traces. | ||
| bool temporal_failure_responses = 1; | ||
| } | ||
|
|
||
| // Headers extracted from the original request in the Temporal frontend. | ||
| // When using Nexus over HTTP, this includes the request's HTTP headers ignoring multiple values. | ||
| map<string, string> header = 1; | ||
|
|
@@ -86,6 +93,8 @@ message Request { | |
| // aip.dev/not-precedent: Not following linter rules. --) | ||
| google.protobuf.Timestamp scheduled_time = 2; | ||
|
|
||
| Capabilities capabilities = 100; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Who is responsible for setting this? I assume the server correct? So is this Nexus
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either of these are constructed only by the server. No strong opinion on where this should go.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of being on the poll response myself, but I similarly do not have a strong opinion |
||
|
|
||
| oneof variant { | ||
| StartOperationRequest start_operation = 3; | ||
| CancelOperationRequest cancel_operation = 4; | ||
|
|
@@ -114,6 +123,9 @@ message StartOperationResponse { | |
| Async async_success = 2; | ||
| // The operation completed unsuccessfully (failed or canceled). | ||
| UnsuccessfulOperationError operation_error = 3; | ||
| // The operation completed unsuccessfully (failed or canceled). | ||
| // Failure object must contain a NexusSDKOperationFailureInfo object. | ||
| temporal.api.failure.v1.Failure failure = 4; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May want to say if this is set, the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's already a oneof so that's not possible. |
||
| } | ||
| } | ||
|
|
||
|
|
||
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.
What is "state" here? May need just a quick comment about what's accepted or what it refers to.
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.
It's
failed | canceledper the Nexus spec. We leave it as a string to allow adding statuses on the Nexus side without requiring a Temporal upgrade.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.
👍 Worth adding a comment here saying this is an enum defined in the Nexus spec