Add Links to HistoryEvent #448
Conversation
| @@ -197,6 +197,8 @@ message StartWorkflowExecutionRequest { | |||
| // for use by user interfaces to display the fixed as-of-start summary and details of the | |||
| // workflow. | |||
| temporal.api.sdk.v1.UserMetadata user_metadata = 23; | |||
| // Links to be associated with the workflow. | |||
| repeated temporal.api.common.v1.Link links = 24; | |||
There was a problem hiding this comment.
Can you confirm this will end up on the started event? What about schedule workflow? What about child workflow? What about signal with start? We should try to make all ways of workflow starting have the same options.
We spent plenty of effort making sure user metadata was propagated in these same kinds of situations. Is everyone sure they don't want to use the user metadata propagation that already exists and just put this inside user metadata?
There was a problem hiding this comment.
Yes, it's added here in the server PR: https://github.com/temporalio/temporal/pull/6420/files#diff-c0867c0ff803c196fa7bc72a15d01812b84ae90eb8b40e71de4ee11d807cfad3R165-R167
At this moment, we're only considering the case of starting new workflow.
Hm, do we want the links to propagate to child workflow, scheduled workflows, etc? I don't think it really make sense.
There was a problem hiding this comment.
IMO we should be consistent in the options like these for all ways of starting a workflow. There are 4 main ones:
- Start workflow
- Signal with start workflow
- Schedule workflow
- Start child workflow
It is important IMO, unless there's a good reason not to in certain cases, for these to all accept the same options.
I don't think it really make sense.
This is a general purpose link option, we shouldn't assume how it'll be used. Same type of thing with user metadata. In fact, arguably this is user metadata and should just be on that message, but that was rejected.
There was a problem hiding this comment.
I agree we should have the option to link in all of these methods. Not blocking this PR but I'd at least want an issue to add it later.
rodrigozhou
force-pushed
the
rodrigozhou/nexus-links
branch
3 times, most recently
from
7abe07e
to
de0aa27
Compare
| @@ -52,6 +52,12 @@ message UnsuccessfulOperationError { | |||
| Failure failure = 2; | |||
| } | |||
|
|
|||
| message Link { | |||
| // See https://github.com/nexus-rpc/api/blob/main/SPEC.md#links. | |||
There was a problem hiding this comment.
This URL does not work. Where can I read about this arbitrary byte array and string type?
There was a problem hiding this comment.
It was discussed in an internal document, the public doc should be updated too.
rodrigozhou
force-pushed
the
rodrigozhou/nexus-links
branch
from
de0aa27
to
a7f2a24
Compare
What changed?
Created
Linkstruct, and added toHistoryEventand Nexus StartOperation request/responseWhy?
Support bi-directional links for Nexus calls.
Breaking changes
Server PR