Commit b64b7ab
Replace
_**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._
<!-- Describe what has changed in this PR -->
- Replace SuggestContinueAsNew +
SuggestContinueAsNewReasonTargetVersionChanged with new
TargetVersionChanged flag
- Add new `target_worker_deployment_version_changed` flag to be sent in
WorkflowTaskStartedEvent and exposed next to `continue-as-new-suggested`
in the workflow info.
<!-- Tell your future self why have you made these changes -->
**Why?**
Setting `SuggestContinueAsNew=true` for Pinned workflows whenever their
is a new Target Version available for that workflow causes Pinned
workflows to hit that condition much more frequently than they expect.
Users who are currently doing: `if workflow_info.suggestContinueAsNew{
do continue-as-new }` in their Pinned workflow code would need to change
that code to protect themselves from running into an infinite-CaN-loop,
because the default CaN behavior for a Pinned workflow is to stay
Pinned.
We should not force users to protect themselves from such a situation.
Because upgrading on continue-as-new is opt-in, receiving the suggestion
to continue-as-new-onto-new-target-version should be opt-in as well. If
people are forced to check the new `suggest-continue-as-new-reasons`
field to "opt out," that is unsafe, because inevitably some people will
forget to do so or misunderstand, and then get hit by this unexpected
footgun.
Much safer and still ergonomical to let upgrade-on-can be opt-in on both
fronts, as proposed here. With this change, the people who are currently
doing `if workflow_info.suggestContinueAsNew{ do continue-as-new }`
won't see any change in semantics, regardless of their versioning
behavior.
People who consciously know that they want to do upgrade-on-can /
Trampolining will have to change their CaN options anyway, so it's easy
enough to teach them to pay attention to this new
`TargetWorkerDeploymentVersionChanged` flag.
<!-- Are there any breaking changes on binary or code level? -->
**I am proposing to completely remove the "new target version" reason
from the can-suggested reasons list. Some SDK PRs use it, and they
should stop. We could also simply deprecate the reason and just tell
people that it's not used anymore, but I'd prefer to flat out delete it.
Server is already patched to not send that flag by default.**
<!-- If this breaks the Server, please provide the Server PR to merge
right after this PR was merged. -->
**Server PR**
temporalio/temporal#9239
---------
Co-authored-by: Shivam Saraf <[email protected]>SuggestContinueAsNew + SuggestContinueAsNewReasonTargetVersionChanged with new TargetVersionChanged flag (#709)1 parent 815022c commit b64b7ab
6 files changed
Lines changed: 27 additions & 24 deletions
File tree
- enums/v1
- history/v1
- proto
- temporalproto/openapi
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
0 commit comments