Skip to content
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 unset_versioning_override to WorkflowExecutionOptionsUpdatedEventAttributes #516

Merged
merged 4 commits into from
Jan 18, 2025

Conversation

carlydf
Copy link
Contributor

@carlydf carlydf commented Jan 16, 2025

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:

Add unset_versioning_override to WorkflowExecutionOptionsUpdatedEventAttributes

Why:

So that users of this event don't need to load VersioningOverride from mutable state every time they create this event.
This change was prompted because the event is now being used for non-version-override-related things, and I received feedback that it is inefficient / awkward / error-prone to have to load and pass in the current versioning override every time anyone writes to this event.

Now, a nil Versioning Override in this event means "no change" instead of "remove".
This reduces the chance that someone accidentally unsets an override in the future, and also is more efficient.
We've discussed this change internally in the server team and are ok with changing the meaning of this history event, because it is such a small change and the scope of impact is small (pre-release versioning users who have unset a versioning override and are building mutable state from that history).

Breaking changes?

Now, a nil Versioning Override in this event means "no change" instead of "remove".
If an event exists with the previous meaning and the mutable state is rebuilt, the Versioning Override would not be removed.
But the chance of that happening is very low.

temporalio/temporal#7091

@carlydf carlydf requested review from a team as code owners January 16, 2025 00:10
@carlydf carlydf marked this pull request as draft January 16, 2025 00:11
@carlydf carlydf marked this pull request as ready for review January 16, 2025 00:34
Copy link
Contributor

@antlai-temporal antlai-temporal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is backwards incompatible, but as described, it should not affect many workflows...

@carlydf carlydf merged commit 71e068d into master Jan 18, 2025
4 checks passed
@carlydf carlydf deleted the cdf/updateoptions-event branch January 18, 2025 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants