Add support for NexusHandler callbacks - #863
Open
chrsmith wants to merge 6 commits into
Open
Conversation
**What changed?** **Why?** There are two changes in this PR, **both breaking changes**. (But since the feature is under development, this won't impact users in any way.) (1) Remove the `callbackpb.CallbackOutcome` proto, and instead move the `success`/`failure` fields directly into `callbackpb.CallbackInfo`. Do the same for `OnCompleteRequest`, removing the `OnCompleteRequest.Outcome` message. It's extremely unlikely we would ever add a new variant for the callback's result, and having the extra layer of indirection made the SDK and server-side code cumbersome. (2) Have `OnCompleteRequest.result` variant `success` be a `commonpb.Payload` instead of a `commonpb.Payloads`. The Temporal API makes it look as if a Workflow can return multiple values. (i.e. a `commonpb.Payloads`.) But in reality, the SDK only ever deals with a single value (`commonpb.Payload`). And the extra layer of indirection via `commonpb.Payloads` also made the SDK and server-side code more cumbersome. **Breaking changes** Yes. But this is still a WIP feature branch. The only person being broken is, alas, @chrsmith .
⚠️ This is to be merged into the `feature/worker-callbacks` branch, and not `main`. Only after the feature is complete will that branch be rebased and merged into `main`. --- This PR makes three changes, all so that resources spanwed from the invocation of a worker callback can be linked correctly. (1) Remove the `Link_NexusOperationCallback` variant with a more general `Link_Callback` proto Previously we were scoping the feature to only be applicable for SANO callbacks. But if we are going to support worker callbacks for any async operation, having a general link type (that uses the existing [Execution proto](https://github.com/temporalio/api/blob/0066de621239ca9ddc6c976e091e27a6bc474752/temporal/api/common/v1/message.proto#L73-L77)) will avoid needing to create additional link variants in the future. (2) Add a `callbackpb.CallbackInfo::request_id` field This type is used in the `Describe-` operations for standalone Activities and standalone Nexus operations. Without it, there would be no way to determine _which_ completion callback is being referred to. (Instead, we couldn't be any more accurate than to have the link point to "one of these N" callbacks.) (3) Add `workflowpb.CallbackInfo::{request_id, result}` The `workflowpb` namespace forked rather than embedded the `callbackpb.CallbackInfo` message. The changes here add the missing fields, so that `DescribeWorkflowExecution` can disambiguate callbacks as well. (In addition to carrying the result of those callbacks.) **Why?** With these changes, the server will be able to properly cross-link resources spawned from completion callbacks. On the Caller-side, any resources spawned from the completion callbacks would be available on the `commonpb.Callback::links` field. (*) ```graphql query GetSpawnedResourceLinks(workflowID: string { DescribeWorkflowExecution(workflowID) { completion_callbacks { callback { links } } } } ``` > (*) Only the resources _initially_ created from the worker callback invocation will be present. e.g. the Workflow that backs an asynchronous Nexus handler. It would not contain links for any subsequent resources created. On the Handler-side, a single `Link_Callback` would be supplied to the Nexus handler receiving the worker callback. (This would be in the form of a `nexuspb.Link`.) **Breaking changes** Yes, this PR contains breaking proto changes. However, in the context of a PR into a long-lived feature branch for an unshipped feature this is safe. (The protos haven't ever been persisted by a production service.) **Server PR** It isn't out yet, but will be stacked on top of this: temporalio/temporal#11589
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
Adds protobufs to support a new variant of
commonpb.CallbackcalledNexusHandler.This also includes additional types to support the new type of callbacks being exposed from the server:
commonpb.Link_Callback, to distinguish links to a particular callback on a resource.temporal.api.notificationservice.v1.OnComplete{ Request, Response }protos, which are the envelope types that will be used sent to Nexus handlers expecting to receive aNexusHandlercallback.StartNexusOperationExecutionRequestto allow attaching callbacks to SANO executions.DescribeNexusOperationExecutionResponse(andtemporal.api.nexusoperation.v1) to retrieve the state of the callbacks attached to a SANO.Why?
The end goal is to provide a streamlined way for adding completion handlers to operations. It's something that in specific scenarios would be more costly to do in other ways.
Breaking changes
None. These fields are purely additive.
Server PR
The server-side support for these new protos are in a set of stacked PRs still being reviewed/developed.
Starting with temporalio/temporal#11380 and ending with temporalio/temporal#11589.