Add Standalone Callbacks (rebase)#774
Conversation
chrsmith
changed the title
chrsmith
force-pushed
the
chrsmith/standalone-callbacks
branch
3 times, most recently
from
a5588a7 to
1064b19
Compare
chrsmith
force-pushed
the
chrsmith/standalone-callbacks
branch
2 times, most recently
from
89e0db0 to
cd67565
Compare
chrsmith
changed the title
| temporal.api.enums.v1.CallbackState state = 5; | ||
|
|
||
| // The number of attempts made to deliver the callback. | ||
| // This number represents a minimum bound since the attempt is incremented after the callback request completes. |
There was a problem hiding this comment.
We should take a pass at this docstring both here and for Nexus ops. I changed the behavior for Nexus to start from attempt 1 and have a better explanation here:
| string blocked_reason = 11; | ||
|
|
||
| // Time when the callback transitioned to a terminal state. | ||
| google.protobuf.Timestamp close_time = 12; |
There was a problem hiding this comment.
nit: could you put this next to create_time? Logically those should be grouped.
| string url = 1; | ||
| // Header to attach to callback request. | ||
| map<string, string> header = 2; | ||
| // Standard token to use for identifying the callback, used for completion. |
There was a problem hiding this comment.
Maybe put a comment here that implementations should also populate the token header field for compatibility with older servers?
| // Callback has failed. | ||
| CALLBACK_EXECUTION_STATUS_FAILED = 3; | ||
| // Callback was terminated via TerminateCallbackExecution. | ||
| CALLBACK_EXECUTION_STATUS_TERMINATED = 4; |
There was a problem hiding this comment.
If the callback times out, we just use the STATUS_FAILED state, and set the TimeoutFailureInfo field in the terminal failure proto.
But I'm assuming you'd like to add it, and I cannot think of a compelling argument not to. 😄
| // Callback is blocked (eg: by circuit breaker). | ||
| CALLBACK_STATE_BLOCKED = 6; | ||
| // Callback was terminated via TerminateCallbackExecution. Only possible for standalone callbacks. | ||
| CALLBACK_STATE_TERMINATED = 7; |
| message CallbackExecutionAlreadyStartedFailure { | ||
| string start_request_id = 1; | ||
| string run_id = 2; | ||
| string callback_id = 3; |
There was a problem hiding this comment.
The other already started failures don't include the business ID, let's not add that here.
| map<string, string> callback_header = 6; | ||
| // Links contain caller information and can be attached to the operations started by the handler. | ||
| repeated Link links = 7; | ||
| // Callback token, to uniquely identify the callback as applicable. |
There was a problem hiding this comment.
nit: document the semantics of when to use this field and when to populate the header value.
| // Identifier for the callback | ||
| string callback_id = 2; | ||
| // Run ID of the callback execution to describe. If empty, the latest run will be described. | ||
| string run_id = 3; |
There was a problem hiding this comment.
Can you check what the semantics of a request with an empty run ID with a long poll token? Is that valid in the SAA code?
There was a problem hiding this comment.
That comment is incorrect. Looking at the implementation and doc comments for SAA, it checks that run_id is present if long_poll_token is set. And it's also called out in the doc comment. Will fix.
|
|
||
| message DescribeCallbackExecutionRequest { | ||
| string namespace = 1; | ||
| // Identifier for the callback |
There was a problem hiding this comment.
nit: All other fields have a punctuation. I see this is missing in other messages for the callback_id field too.
| // Identifier for the callback | |
| // Identifier for the callback. |
| int32 page_size = 2; | ||
| // Token returned in ListCallbackExecutionsResponse. | ||
| bytes next_page_token = 3; | ||
| // Visibility query, see https://docs.temporal.io/list-filter for the syntax. |
There was a problem hiding this comment.
Please document which search attributes are valid here (and for the count API).
chrsmith
force-pushed
the
chrsmith/standalone-callbacks
branch
from
cd67565 to
7c4b67e
Compare
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Left just one small comment.
Co-authored-by: Roey Berman <roey@temporal.io>
2 participants
What changed?
Add the API for standalone callbacks. (AKA Manual Completions for Nexus.)
Why?
Standalone callbacks support connecting non-Temporal services to Nexus. e.g. supporting a way for, say, a Slackbot directly complete a Nexus operation rather than needing to add an intermediary workflow.
Breaking changes
None, this is purely additive.
Server PR
These changes also require updates in the
api-gopackage as well, since the use ofoneofrequires some minor codegen tweaks.temporalio/api-go#255
The server-side changes are here:
temporalio/temporal#10192