API surface area for worker callbacks

[chrsmith]

⚠️ This isn't meant to be merged any time soon. This is just part of the worker callbacks implementation. After the code review, I'll replace the feature/worker-callbacks branch with this.

---

What changed?

Adds the API surface area for the WIP worker callbacks feature.

Specifically, the changes are:

1. Add a completion_callbacks field to StartNexusOperationExecutionRequest. (How worker callbacks will be sent to the server.)
2. Introduce a new Worker variant of commonpb.Callback. (How worker callbacks will be encoded.)
3. Add a new notificationservice/v1/request_response.proto to define the input/output shape for worker callbacks. (The payload that will be delivered to workers.)

Why?

Today it is cumbersome to durably invoke Nexus operations and execute logic based on their results. "Worker callbacks" is a capability being developed that would allow developers to attach a Nexus operation to a SANO. Later, when the SANO reaches a terminal state, the attached Nexus operation would be invoked.

Breaking changes

None.

Server PR

TBD. The current prototype (based on the feature/worker-callbacks branch in this repo) can be found at:
https://github.com/temporalio/temporal/compare/feature/worker-callbacks?expand=1

[bergundy]

You're missing a new link definition to a callback on an execution.

[bergundy]

nit: You can also mention which dynamic config controls this limit for self-hosters.

[chrsmith]

Good call. I haven't added that on the server-side yet, but will file a ticket and make a point to update this comment when that dynamic config value actually exists.

I was thinking we'd go with a new dynamic config value like limit.callbackContext.warn and limit.callbackContext.error. (Patterned after limit.blobSize.*, limit.memoSize.*, etc.)

[bergundy]

nit: we call this field task_queue everywhere, it will generate better across languages with the underscore. Rename to task_queue_name.

[chrsmith]

Asking because I'm not sure: is it a problem that we aren't using the proper TaskQueue proto type here?

This is the full proto definition. We'd be missing out on the kind and normal_name of task queue. And just assuming it's TASK_QUEUE_KIND_NORMAL. (And never TASK_QUEUE_KIND_WORKER_COMMANDS or TASK_QUEUE_KIND_STICKY.)

Could that be a problem down the road?

[bergundy]

Let's just have a TODO to see if we want to allow customizing this from the get-go or not. The default can be on the server and it's easier to add fields later than remove them.

[chrsmith]

I'm inclined to keep this as-is, and allow for callbacks to specify an any Nexus service and operation. By fixing those values it makes using worker callbacks much more painful. (Since you need to spin up multiple Temporal workers, with different versions of the NotificationService listening to different task queues.)

I could be convinced otherwise, but I'd rather start with this and see if we can convince ourselves that it really isn't needed than leave it as a TODO. (e.g. let's bring this up during the the Cross-SDK review.)

[bergundy]

As discussed in person, I would leave this out for now. When we add support for attaching callbacks to other primitives, I am not convinced we will want to provide information per primitive here.

The link + source context get you quite a bit of information already.

If we do decide to keep this for the first iteration, I would name this differently because it's not necessarily an operation in the nexus sense, it's a primitive or component within a primitive (workflow run, workflow update, activity, schedule, nexus operation).

[chrsmith]

I could be convinced otherwise, but I don't think the "link + source context" is sufficient. (But it depends on what the link actually is, and the information that's available.)

The next rev of this PR is specifically about exposing the CallbackExecutionInfo and explaining why I don't think we should be adding links. (So let's resolve those bigger topics, before we get into the details of this particular proto and the link it would be sent with.)

[bergundy]

You'll need to add the full attachment support that we have for request IDs and on conflict options for workflows and activities.

[chrsmith]

Would you mind elaborating on what you mean by this? What do you mean by "full attachment" here?

I'm patterning this after the way we added completion callbacks to StartActivityExecutionRequest and StartWorkflowExecutionRequest. I see in both of those other request types there are id_conflict_policy / id_reuse_policy fields. But StartNexusOperationExecutionRequest has similar fields as well.

Are you saying that each individual commonpb.Callbacks should have its own (potentially user-supplied) callback_id ? And ID Conflict/Reuse policies supplied too? That doesn't make sense to me. Since if the SANO is accepted by Temporal, the callbacks should be accepted as well. (And I don't see any sort of UpdateNexusOperationExecution-type command, that would allow us to merge/update callbacks?)

[chrsmith]

@bergundy Thanks for the review, please take another look.

• Expose a new CallbackExecutionInfo proto from DescribeNexusOperationExecutionResponse. (It's a subset of what was going to be added for Manual Completion in Add Standalone Callbacks (rebase) #774.)
• Add a new Callback variant of Link.