Set TemporalChangeVersion when workflow version is updated#2464
Merged
Quinn-With-Two-Ns merged 5 commits intotemporalio:masterfrom May 28, 2025
Merged
Set TemporalChangeVersion when workflow version is updated#2464Quinn-With-Two-Ns merged 5 commits intotemporalio:masterfrom
Quinn-With-Two-Ns merged 5 commits intotemporalio:masterfrom
Conversation
Quinn-With-Two-Ns
force-pushed
the
issue-587
branch
from
e7ec581 to
e77c71c
Compare
Quinn-With-Two-Ns
changed the title
cretz
reviewed
May 14, 2025
Comment on lines
+111
to
+93
| details.put( | ||
| UPSERT_VERSION_SA_KEY, | ||
| DefaultDataConverter.STANDARD_INSTANCE.toPayloads(upsertVersionSA).get()); |
Contributor
There was a problem hiding this comment.
Can you help me understand the changing of the marker value here. Do other SDKs do this and/or are we using it as a flag of sorts?
Contributor
Author
There was a problem hiding this comment.
Yes Go does the same thing, it tells the SDK if it should expect a search attribute after the marker.
| } | ||
| if (shouldSkipUpsertVersionSA) { | ||
| if (handleNonMatchingUpsertSearchAttribute(event)) { | ||
| shouldSkipUpsertVersionSA = false; |
Contributor
There was a problem hiding this comment.
Is this bool only valid for this one event? Does setting it back to false here make a difference, or is it just to be safe?
Contributor
Author
There was a problem hiding this comment.
Yes it is only valid for one event
cretz
approved these changes
May 14, 2025
| * with running workflows. However, if this options is enabled, it is not safe to rollback to a | ||
| * previous version of the SDK that does not support this option. | ||
| * | ||
| * <p>The default value is false. This may change in future releases. |
Contributor
There was a problem hiding this comment.
Wonder if we need an issue to track enabling this by default after a release or two
Sushisource
approved these changes
May 15, 2025
Member
Sushisource
left a comment
Sushisource
left a comment
There was a problem hiding this comment.
I believe this does indeed match Core
temporal-sdk/src/main/java/io/temporal/internal/replay/ReplayWorkflowContextImpl.java
Outdated
Show resolved
Hide resolved
Quinn-With-Two-Ns
force-pushed
the
issue-587
branch
2 times, most recently
from
8b49685 to
3bd0dd6
Compare
Quinn-With-Two-Ns
force-pushed
the
issue-587
branch
from
3bd0dd6 to
35d1dc8
Compare
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Set TemporalChangeVersion when workflow version is updated
This PR adds an option to
WorkflowImplementationOptionsthat if set to true will causeGetVersionto automatically upsertTemporalChangeVersionwith a list of version. The reason why this is behind a flag is to allow for safe rollback and to help users who are already manually upserting a search attribute migrate.Note for reviewers: I still need to fix the parametrized test to cover both the flag being enabled and disable, and add more replay tests, but all the tests I intended to add are here otherwise. I welcome suggestions for more test coverage
closes #587