Add Callbacks and Links to Workflow Update#742
Add Callbacks and Links to Workflow Update#742Quinn-With-Two-Ns wants to merge 10 commits intotemporalio:masterfrom
Conversation
| // Ignored if nil. | ||
| temporal.api.common.v1.Priority priority = 6; | ||
|
|
||
| map<string, WorkflowUpdateOptionsUpdate> workflow_update_options = 7; |
There was a problem hiding this comment.
Is the key here the update ID? I slightly prefer putting the update ID in the WorkflowUpdateOptionsUpdate event so it's clear what the field just by the name. That would turn this field's type to repeated WorkflowUpdateOptionsUpdate.
There was a problem hiding this comment.
Yeah I thought we would need to index into this but we never ended up needing to
| // update. | ||
| temporal.api.update.v1.Request accepted_request = 4; | ||
| // Completion callbacks attached when this update was started. | ||
| // TODO(quinn) maybe not needed |
There was a problem hiding this comment.
Looks like it's needed? My only concern is that the event time here is for when the update was accepted but there's no indication of when the callback was received for attachment.
There was a problem hiding this comment.
We currently don't keep any time for when the update was received, so if we don't care about when the update was received I don't think a callback changes that.
There was a problem hiding this comment.
We already attach the callback to the request so we can remove the callback here.
There was a problem hiding this comment.
Agree knowing the request time could be useful and is a separate concern.
| string namespace = 1; | ||
| string workflow_id = 2; | ||
| string run_id = 3; | ||
| temporal.api.enums.v1.LinkReason reason = 4; |
There was a problem hiding this comment.
I kinda prefer a string here, it's a bit more overhead but doesn't require translating enums to strings and can flow newer values through older SDK and server versions that don't have up-to-date protos.
There was a problem hiding this comment.
I don't have a strong opinion here. I thought enums were just easier to keep consistent across the SDK, Server and UI.
2 participants
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 Callbacks and Links to Workflow Update.
Why?
Will allow starting async updates from a Nexus Operation Handler
Breaking changes
Server PR
temporalio/temporal#9614