-
Notifications
You must be signed in to change notification settings - Fork 86
Use Temporal failures in Nexus APIs #682
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
Conversation
bergundy
left a comment
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.
LGTM, assuming that this won't be merged until it's proven to work.
temporal/api/nexus/v1/message.proto
Outdated
| // The operation completed unsuccessfully (failed or canceled). | ||
| UnsuccessfulOperationError operation_error = 3; |
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.
Should we mark this as deprecated?
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.
Yes!
| Async async_success = 2; | ||
| // The operation completed unsuccessfully (failed or canceled). | ||
| UnsuccessfulOperationError operation_error = 3; | ||
| // Deprecated. Use the failure variant instead. |
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.
nit:
| // Deprecated. Use the failure variant instead. | |
| // Deprecated. Use the failure variant instead if temporal_failure_responses was true on the request. |
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.
Ah, I saw your nit too late.
What changed?
Added Temporal failure representations for Nexus errors.
Why?
Consistency with other Temporal APIs and to make it easier to handle payloads.
Breaking changes
Not explicitly, but server and SDK will need to be able to handle both error formats based on
capabilitiesfield.Server PR
temporalio/temporal#8799