-
Notifications
You must be signed in to change notification settings - Fork 66
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
Add Links to HistoryEvent #448
Changes from all commits
19bdade
f93befa
46af2c4
c4f1073
a7f2a24
16292c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO we should be consistent in the options like these for all ways of starting a workflow. There are 4 main ones:
It is important IMO, unless there's a good reason not to in certain cases, for these to all accept the same options.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
message StartWorkflowExecutionResponse { | ||
rodrigozhou marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
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 URL does not work. Where can I read about this arbitrary byte array and string type?
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.
It was discussed in an internal document, the public doc should be updated too.