Add link field to Callback

[rodrigozhou]

READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.

What changed?
Add links field to Callback.

Why?
Be able to explicitly associate a callback with links.

Breaking changes

Server PR

[cretz]

This is a bit confusing in a common object. I am not sure we should assume uses of this common object are for starting workflows or use indices of something in a completely separate area.

Can you clarify the "Be able to explicitly associate a callback with a link" in the description a bit more? That says why we make this API, but is there anywhere that says why we have to associate callbacks with links? If possible, would also like to see an expected start request where you don't want to associate a callback with a link and one where you do just to help understand what start requests look like in these scenarios (sorry, hard for me to picture).

[bergundy]

This is more about a start request with multiple callbacks and links (which is something that Temporal supports but isn't part of the Nexus spec).

Agree we can be a bit more descriptive.

We may also want to use a separate field on StartWorkflowExecutionRequest instead of embedding the index in the callback (might be overkill but figured I'd bring it up for consideration):

message CallbackLinkAssociation {
  int32 callback_index = 1;
  int32 link_index = 2;
}

message StartWorkflowExecutionRequest {
  // ...
  repeated CallbackLinkAssociation;
}

I would also mention in the comments that there should be a one to one mapping between a link and a callback and that associating multiple callbacks with the same link would not be supported.

[rodrigozhou]

Do we need a struct? Any reason not to be a simple map?

message StartWorkflowExecutionRequest {
	// ...
	map<int32, int32> callback_link_association;
}

[cretz]

Will all of our current use of callbacks and links be associating them with each other, or are there start calls we are making today where we are adding links not associated with the callbacks?

From my earlier comment: Can you clarify the "Be able to explicitly associate a callback with a link" in the description a bit more? That says why we make this API, but is there anywhere that says why we have to associate callbacks with links? If possible, would also like to see an expected start request where you don't want to associate a callback with a link and one where you do just to help understand what start requests look like in these scenarios (sorry, hard for me to picture).

I am specifically looking to understand why users need callbacks associated with links and specifically looking for why such association needs to be so flexible. I am also wondering why both links and callbacks cannot be associated with the same thing themselves (e.g. a workflow) and instead have to be associated with each other even though they are seemingly separate things.

[rodrigozhou]

The start request allows you to set multiple callbacks and multiple links. The idea is that each callback could be associated with a link. Say, there is a callback to notify a caller workflow. The callback itself has no information about the workflow, but a link does. So, by associating a callback with a link, we're basically creating a way for the user to go the workflow that it will notify when completed.

[cretz]

I am a bit confused on a few fronts...

The start request allows you to set multiple callbacks and multiple links

Right, but is there a use case today where you need to choose which callback is for which link, or can we blindly assume that every callback in the start request is associated with every link in the start request? Is there a current use case to need manual mapping, or are you just assuming there may be one day?

The callback itself has no information about the workflow, but a link does

It sounds like you want a callback associated with a start request, not just a link

The callback itself has no information about the workflow, but a link does

The callback itself should contain whatever info it needs to be provided when it is invoked. It's not just about information about a workflow. There are plenty of things a callback may want to know about itself when it gets called back. Link isn't unique IMO. Usually this is metadata or header or whatever that is set by the callback creator that is provided upon invocation.

[rodrigozhou]

Right, but is there a use case today where you need to choose which callback is for which link, or can we blindly assume that every callback in the start request is associated with every link in the start request? Is there a current use case to need manual mapping, or are you just assuming there may be one day?

Yes, there is. If the workflow is continued-as-new, we don't call the callbacks, and they are copied over to the new workflow. In this case, the WorkflowExecutionStarted event will potentially have multiple callbacks and links. Also, there is no requirement that for every callback, you have to add a link, or vice-versa.

The callback itself should contain whatever info it needs to be provided when it is invoked. It's not just about information about a workflow. There are plenty of things a callback may want to know about itself when it gets called back. Link isn't unique IMO. Usually this is metadata or header or whatever that is set by the callback creator that is provided upon invocation.

Yes, the callback has a token that needs to added to the header when called, but there is no requirement that the callback (URI or metadata) is readable by the handler. We have links for this.

[bergundy]

There is another alternative where we embed the Link in the Callback object. This would be the most intuitive and would guarantee the 1:1 association.

The only issue with that is that we would want to still send links in the old way to older servers, to get around that we could:

1. Use server capabilities to determine where to attach the links.
2. Send links in both fields (for a long time) and let the server dedupe.

I'm now leaning towards option two.
The logic for deduping links should be fairly straightforward.

[cretz]

Yes, there is. If the workflow is continued-as-new, we don't call the callbacks, and they are copied over to the new workflow.

I am referring to StartWorkflowExecutionRequest specifically which I assume is what this should narrowly apply to? I think it's very confusing to have this mapping refer to an index in some object we can't see.

Agreed, it sounds like callbacks have links rather than referring to them in some external place.

[rodrigozhou]

Alright, I updated the PR and added a link field to Callback.

[cretz]

Doesn't Nexus spec support multiple links on a callback? Should the Temporal server allow that (especially if one day we let users set their own links)?

[bergundy]

It does but we will choose only the first one to associate with the callback, the rest will be linked still but not associated directly.

[bergundy]

I'd be okay to also attach all of the links to the callback and let the UI decide what to display. The current option was chosen to allow the server/SDK to choose the association and reduce the amount of logic needed in the UI. I don't see a use case where we render multiple links for a given callback.

[cretz]

Why not support multiple links on the callback model even if you only allow one at runtime today? Doesn't that more accurately reflect the possibilities of the model Nexus side? Or is it believed that there is no way we'd ever want multiple links for this common "callback" object (that is not specific to either Nexus or start workflow)?

[rodrigozhou]

Changed to support multiple links. We will let UI decide what to display.

[cretz]

Thanks! Makes total sense that we have no expectation of it ever being more than one at this time.