-
Notifications
You must be signed in to change notification settings - Fork 78
[DO NOT MERGE] Workflow service APIs for durable external Nexus callers #655
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
| // If the request is denied, the server returns a `NexusOperationAlreadyStartedFailure` error. | ||
| // | ||
| // See `NexusCallerOperationRefConflictPolicy` for handling caller operation ref duplication with a *running* operation. | ||
| enum NexusCallerOperationRefReusePolicy { |
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.
Lifted straight from workflow ID policies
| message CompleteNexusOperationResponse { | ||
| } | ||
|
|
||
| message FetchCallbackResultRequest { |
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.
CompleteNexusOperation is the only handler-side API that is Nexus-specific. I don't think the other callback-related APIs need anything Nexus and can be reused if we want to expose other types of callbacks in the future.
| // A UUID used to deduplicate client requests. | ||
| string request_id = 3; | ||
| // The callback token as provided in the StartOperation call. | ||
| bytes callback_token = 4; |
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.
I decided to use callback_token to index the callback state machines on the handler-side since I thought it would be a little bit cleaner. But I'm not sure what the final form of this token will be. If it is not suitable, we can always revert back to using request_id.
Since callbacks will not be re-run, I don't think we need to copy what we did on the caller side with operation_ref. I think it's also more intuitive to reuse request_id in this case since further actions on the callback will be referencing the original completion request.
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.
You're missing "describe" and "list". Please see the latest API for standalone activities and replicate the approaches taken there.
| } | ||
|
|
||
| // Retrieve the result of a Nexus operation. Optionally specify a wait period to turn this request into a long poll. | ||
| rpc FetchNexusOperationResult (FetchNexusOperationResultRequest) returns (FetchNexusOperationResultResponse) { |
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.
Please align with what @dandavison is doing for standalone activities, where we merged describe and get result.
| // | ||
| // (-- api-linter: core::0127::http-annotation=disabled | ||
| // aip.dev/not-precedent: Nexus has a separately defined HTTP API. --) | ||
| rpc CompleteNexusOperation (CompleteNexusOperationRequest) returns (CompleteNexusOperationResponse) { |
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.
Let's not add for now. This will come at a later stage.
| }; | ||
| } | ||
|
|
||
| // Start an arbitrary length Nexus operation. The result of the operation may be retrieved using the |
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.
There's a lot more to say here on the semantics of durable and direct calls.
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.
You can probably leave out direct though since we don't intend to implement this yet.
| rpc RequestCancelNexusOperation (RequestCancelNexusOperationRequest) returns (RequestCancelNexusOperationResponse) { | ||
| } | ||
|
|
||
| // Retrieve the result of a Nexus operation. Optionally specify a wait period to turn this request into a long poll. |
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.
Need to explain that this is only for durable if we end up doing direct too.
| }; | ||
| } | ||
|
|
||
| // |
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.
Put a blurb here and in the rest of the methods.
| // | ||
| rpc FetchCallbackResult (FetchCallbackResultRequest) returns (FetchCallbackResultResponse) { | ||
| option (google.api.http) = { | ||
| get: "/namespaces/{namespace}/callbacks/{callback_token}/result" | ||
| additional_bindings { | ||
| get: "/api/v1/namespaces/{namespace}/callbacks/{callback_token}/result" | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| // | ||
| rpc TerminateCallback (TerminateCallbackRequest) returns (TerminateCallbackResponse) { | ||
| option (google.api.http) = { | ||
| post: "/namespaces/{namespace}/callbacks/{callback_token}/terminate" | ||
| body: "*" | ||
| additional_bindings { | ||
| post: "/api/v1/namespaces/{namespace}/callbacks/{callback_token}/terminate" | ||
| body: "*" | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| // | ||
| // | ||
| // (-- api-linter: core::0127::http-annotation=disabled | ||
| // aip.dev/not-precedent: Callback deletion not exposed to HTTP, users should use terminate. --) | ||
| rpc DeleteCallback (DeleteCallbackRequest) returns (DeleteCallbackResponse) { | ||
| } |
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.
Let's leave this out for this stage of the project.
| // Identifies a specific Nexus operation in the caller (i.e. initiating) namespace ONLY. This reference is NOT sent to | ||
| // the operation handler which may be in a different namespace. Because run_id is a UUID, a caller operation reference | ||
| // is unique within the calling namespace. | ||
| message CallerOperationReference { |
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.
Don't create this struct, we decided against this for standalone activities and would rather just flatten these two items into the requests and responses.
| // If the request is denied, the server returns a `NexusOperationAlreadyStartedFailure` error. | ||
| // | ||
| // See `NexusCallerOperationRefConflictPolicy` for handling caller operation ref duplication with a *running* operation. | ||
| enum NexusCallerOperationRefReusePolicy { |
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.
You'll need to rename this to NexusDurableOperationIdReusePolicy (I think call is redundant).
| // Note that it is *never* valid to have two actively running instances of the same caller operation ref. | ||
| // | ||
| // See `NexusCallerOperationRefReusePolicy` for handling operation ref duplication with a *completed* operation. | ||
| enum NexusCallerOperationRefConflictPolicy { |
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.
Same here, rename.
| // Callback URL to call upon completion if the started operation is async. | ||
| string callback_url = 14; | ||
| // Serialized object containing information about how to route the callback and metadata to send with it. | ||
| bytes callback_context = 15; | ||
|
|
||
| // Links contain caller information and can be attached to the operations started by the handler. | ||
| repeated temporal.api.nexus.v1.Link links = 16; |
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.
This is out of scope for now.
You also forgot to add search attributes, schedule to close timeout and cancellation type.
|
The changes were significant enough that I decided to open a new PR: #659 |
What changed?
Added new workflow service APIs to support durable external Nexus callers
Why?
New feature
Breaking changes
No