Inherit pinned Version from parent #7245
Conversation
29b795c to
f6576e8
Compare
962610c to
ffd2fe4
Compare
service/history/historybuilder/history_builder_categorization_test.go
Outdated
Show resolved
Hide resolved
| var parentPinnedOverride *workflowpb.VersioningOverride | ||
| if attributes.TaskQueue.GetName() == mutableState.GetExecutionInfo().GetTaskQueue() { | ||
| // TODO (shahab): also inherit when the child TQ is different, but in the same Version | ||
| if mutableState.GetEffectiveVersioningBehavior() == enumspb.VERSIONING_BEHAVIOR_PINNED { |
There was a problem hiding this comment.
I'm not convinced that this should be the EffectiveVersioningBehavior. If the parent is unpinned base with a pinned override, I think the child should be unpinned base with a pinned override. Right now, the child would be pinned base with a pinned override.
There was a problem hiding this comment.
If the parent has a pinned override, it doesn't matter because the child will get the pinned override anyways.
Now, the parent is pinned but has an unpinned, then I don't think there is any point to start the child in the same build because the parent is effectively unpinned and hence cannot expect interface compatibility protection.
There was a problem hiding this comment.
If the parent has (base: unpinned, override: pinned) I think it does matter for the child to have (base: unpinned, override: pinned) instead of (base: pinned, override: pinned), because if the user does a batch job to unset the override on a batch that covers all the parents and children of one workflow type, then the parent would become unpinned but the child would remain pinned, which is probably not what the user wants / expects.
There was a problem hiding this comment.
actually, thinking about what Antonio said is making me reconsider this.
The true base pinned-ness or unpinned-ness of the child ultimately comes from how the child wftype is registered in the code.
If the child starts on the pinned override of their unpinned parent (let's say Version A), hopefully, if the user expects the child to be unpinned, the Version A code of the child will say that the child is unpinned. Then, on WFT completion, the child's base behavior will immediately become unpinned, as the user expects.
There was a problem hiding this comment.
I think the way you have it is correct. And now that the comment says "effectively Pinned", the proto is technically accurate as well.
## What changed? <!-- Describe what has changed in this PR --> Child workflows start in parent's pinned Worker Deployment Version. ## Why? <!-- Tell your future self why have you made these changes --> So user do not have to worry about interface compatibility between pinned parents and children. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Added new tests. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> Child workflows start in parent's pinned Worker Deployment Version. ## Why? <!-- Tell your future self why have you made these changes --> So user do not have to worry about interface compatibility between pinned parents and children. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Added new tests. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> Child workflows start in parent's pinned Worker Deployment Version. ## Why? <!-- Tell your future self why have you made these changes --> So user do not have to worry about interface compatibility between pinned parents and children. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Added new tests. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> Child workflows start in parent's pinned Worker Deployment Version. ## Why? <!-- Tell your future self why have you made these changes --> So user do not have to worry about interface compatibility between pinned parents and children. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Added new tests. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
What changed?
Child workflows start in parent's pinned Worker Deployment Version.
Why?
So user do not have to worry about interface compatibility between pinned parents and children.
How did you test it?
Added new tests.
Potential risks
Documentation
Is hotfix candidate?