inherit pinned version and override across CaN when pinned #588
Conversation
| // instead of starting on the Current Version of its Task Queue. | ||
| // This is set only if the new workflow is starting on a Task Queue belonging to the same | ||
| // Worker Deployment Version. | ||
| temporal.api.deployment.v1.WorkerDeploymentVersion previous_pinned_deployment_version = 37; |
There was a problem hiding this comment.
- nit:
continued_from_pinned_deployment_versionfor clarity? - nit: At the cost of sending a redundant identifier, it seems easier to explain/more declarative, and more robust, to just set this always and then let the new workflow decide whether it wants to ignore the information. That way, you're not embedding intent into it, you're just presenting the data.
There was a problem hiding this comment.
(The main related question I have is, have we thought through transition points where the workflow is changing behaviors across code revisions?)
There was a problem hiding this comment.
Here is what I think we want:
CaN: Auto-upgrade -> Pinned
- WF1 is running on version A, where it is labelled auto-upgrade
- WF1 Continues as new, call the new wf WF1.1
- Because WF1 is not Pinned, WF1.1 does not inherit the pinned version, and starts on the current version of its task queue, which is now B. In version B, this WF type is labelled Pinned, so the behavior of WF1.1 becomes Pinned on the first WFT completion send by the version B worker.
- WF1.1 Continues as new, call the new wf WF1.2
- Because WF1.1 is Pinned to B, WF1.2 inherits the pinned version B, and starts on version B*
- The first WFT completion on WF1.2 will come from worker B, which will say this wf type is Pinned, so WF1.2 will remain Pinned.
*if WF1.2 starts on a different task queue from WF1.1, and that other task queue is not in Version B, then WF1.2 will not inherit, and will instead start on the current version of its task queue. This is uncommon.
| bool inherit_build_id = 15; | ||
|
|
||
| // If this is set, the new execution inherits the versioning override of the current execution, |
There was a problem hiding this comment.
Symmetric point here: should we just always set this?
| bool inherit_build_id = 15; | ||
|
|
||
| // If this is set, the new execution inherits the versioning override of the current execution, | ||
| // because the effective behavior of the current execution is pinned. | ||
| temporal.api.workflow.v1.VersioningOverride current_pinned_versioning_override = 16; |
There was a problem hiding this comment.
since this event belongs to the previous run we might not need it at all. That workflow already knows it's pinned version and override.
There was a problem hiding this comment.
yes, the important thing is just that we create the new mutable state correctly
…pinned version of a tree/chain can be inherited on retry
| // of starting on the Current Version of its Task Queue. | ||
| // This is set only if the child workflow is starting on a Task Queue belonging to the same | ||
| // Worker Deployment Version. | ||
| temporal.api.deployment.v1.WorkerDeploymentVersion parent_pinned_deployment_version = 36; |
There was a problem hiding this comment.
Not generally reviewing PR (leaving up to others that know versioning better), but I can I get confirmation that this field is not referenced in any SDK or CLI code? I didn't see it, just wanting to confirm.
There was a problem hiding this comment.
I asked @Sushisource last week and he told me that it was not. I also don't think that it is. This is used internally in the server.
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 fields to continued as new event to enable inheriting pinned version and override across CaN when the effective behavior of the WF is pinned.
This inheritance will not happen if the behavior is anything else (auto-upgrade, pinned-until-continue-as-new, etc)
Why?
To differentiate pinned from trampolining
No
Server PR