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

Make GetVersion more deterministic #1807

Merged
merged 3 commits into from
Jul 17, 2023

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

Fix a possible source of non determinism in GetVersion calls. Since GetVersion can block on a future adding a call to it in a workflow can lead to non determinism so we need to avoid doing so in a backwards compatible way.

I tried to refactor the Version state machine to avoid ever blocking, but found the work too complicated for the benefit since due to backwards compatibility we can never remove the old state machine.

closes #1430

@Quinn-With-Two-Ns Quinn-With-Two-Ns marked this pull request as ready for review July 6, 2023 23:26
@Quinn-With-Two-Ns Quinn-With-Two-Ns requested a review from a team as a code owner July 6, 2023 23:26
Comment on lines 45 to 51
public static SdkFlag GetValue(int _id) {
SdkFlag[] As = SdkFlag.values();
for (int i = 0; i < As.length; i++) {
if (As[i].Compare(_id)) return As[i];
}
return SdkFlag.UNKNOWN;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static SdkFlag GetValue(int _id) {
SdkFlag[] As = SdkFlag.values();
for (int i = 0; i < As.length; i++) {
if (As[i].Compare(_id)) return As[i];
}
return SdkFlag.UNKNOWN;
}
public static Optional<SdkFlag> fromOrdinal(int ordinal) {
if ordinal < 0 || ordinal >= SdkFlag.values().length - 1 {
return Optional.empty();
}
return Optional.of(SdkFlag.values()[ordinal]);
}

Might as well just resolve to always match the index and keep it optional instead of adding an unusable value to the class.

* Changes behaviour of GetVersion to not yield if no previous call existed in history.
*/
SKIP_YIELD_ON_DEFAULT_VERSION(1),
UNKNOWN(Integer.MAX_VALUE);
Copy link
Member

Choose a reason for hiding this comment

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

Why have an unknown flag? Just make the return from GetValue (sic) as Optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UNKNOWN is not the same as an empty optional

Copy link
Member

@cretz cretz Jul 17, 2023

Choose a reason for hiding this comment

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

What I'm saying is it shouldn't be the same. You should only have enumerates for real flags IMO. There's no reason to have all but one enumerate be a real flag IMO. Make it clear to the caller that the flag is unknown, not just that it happens to have another name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think returning UNKNOWN is clear to the caller that the flags meaning is not know. This is how we handle it in the other SDKs that implement flags as well

Copy link
Member

Choose a reason for hiding this comment

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

Eek. I hope we at least don't in TS, Python, and .NET. All those languages have a good way of representing an absent enum without having to work around that some values of the enum are representable in history and some aren't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if it was an absent enum I would use optional, but it really is the lack of knowledge about how to handle a value. I would rather leave it as is in Java since I think it is clearer and more consistent with other SDKs.

Copy link
Member

Choose a reason for hiding this comment

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

I don't agree, but y'all's design choice here. At least it's internal.

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit 6054c31 into temporalio:master Jul 17, 2023
4 checks passed
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.

Workflow.getVersion() could cause NonDeterminsticError with multithreading + timer
2 participants