Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make sure GetVersion never yields #2376

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

Make sure GetVersion never yields

@Quinn-With-Two-Ns Quinn-With-Two-Ns requested a review from a team as a code owner January 17, 2025 23:20
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

Makes sense 👍

I'm assuming this means you decided against doing the whole two-phase thing?

Comment on lines 285 to +287
* @return True if the identifier is not present in history
*/
boolean getVersion(
Integer getVersion(
Copy link
Member

Choose a reason for hiding this comment

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

Docstring needs update

Comment on lines 380 to +382
* @return True if the identifier is not present in history
*/
public boolean getVersion(
public Integer getVersion(
Copy link
Member

Choose a reason for hiding this comment

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

Docstring needs update here too

@@ -75,6 +75,9 @@ public void testGetVersionOutOfOrderFail() {
assertEquals(
NonDeterministicException.class.getName(),
((ApplicationFailure) e.getCause().getCause().getCause()).getType());
assertEquals(
"[TMPRL1100] getVersion call before the existing version marker event. The most probable cause is retroactive addition of a getVersion call with an existing 'changeId'",
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
"[TMPRL1100] getVersion call before the existing version marker event. The most probable cause is retroactive addition of a getVersion call with an existing 'changeId'",
"[TMPRL1100] getVersion called before the existing version marker event. The most probable cause is retroactive addition of a getVersion call with an existing 'changeId'",

@Quinn-With-Two-Ns
Copy link
Contributor Author

I'm assuming this means you decided against doing the whole two-phase thing?

@Sushisource not sure what you mean by "the whole two-phase thing"?

@Sushisource
Copy link
Member

Sushisource commented Jan 21, 2025

I'm assuming this means you decided against doing the whole two-phase thing?

@Sushisource not sure what you mean by "the whole two-phase thing"?

Doing the change in two parts so rolling back one version would work. But, maybe I'm actually getting this mixed up with a different change. Doesn't matter either way.

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit f93910b into temporalio:master Jan 28, 2025
8 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants