Backport to branch(3) : Fix behavior of group commit with one-phase commit #2846

feeblefakie · 2025-07-09T09:20:01Z

This is an automated backport of the following:

Original PR: Fix behavior of group commit with one-phase commit #2832
Backported commit: e7224b9

Please merge this PR after all checks have passed.

gemini-code-assist

Summary of Changes

Hello @feeblefakie, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request backports a critical fix that resolves the behavioral issues when combining the group commit and one-phase commit features. The changes ensure that the internal state of group commit is correctly managed and cleaned up during one-phase commit operations, thereby enabling the stable use of both features together. The accompanying test updates validate this corrected behavior.

Highlights

Enabling One-Phase Commit with Group Commit: This pull request enables the use of one-phase commit alongside the group commit feature, which was previously considered unsupported. This is a significant change allowing for more flexible transaction configurations.
Correct Group Commit State Management: The CommitHandlerWithGroupCommit now explicitly ensures that the group commit state is properly canceled or removed (cancelGroupCommitIfNeeded) during one-phase commit operations. This cleanup occurs both when checking if one-phase commit is possible and when performing the commit, even if exceptions are thrown.
Comprehensive Test Suite Updates: Numerous previously disabled tests related to one-phase commit within the CommitHandlerWithGroupCommitTest suite have been re-enabled. These tests now include assertions to verify that the group commit state is correctly managed and cleaned up after one-phase commit attempts, whether successful or failed.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

The pull request backports a fix related to the behavior of group commit with one-phase commit. The changes include modifications to the CommitHandlerWithGroupCommit class to handle exceptions during one-phase commit and updates to the test classes to reflect the changes in behavior. The code review identified opportunities to reduce redundancy and improve readability.

gemini-code-assist · 2025-07-09T09:21:43Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommit.java

+  boolean canOnePhaseCommit(Snapshot snapshot) throws CommitException {
+    try {
+      return super.canOnePhaseCommit(snapshot);
+    } catch (CommitException e) {
+      cancelGroupCommitIfNeeded(snapshot.getId());
+      throw e;
+    }


Wrapping the call to super.canOnePhaseCommit in a try-catch block and then throwing the caught exception is redundant. You can simplify this by directly throwing the exception after canceling the group commit if needed.

Also, consider logging the exception before re-throwing it to aid in debugging.

Suggested change

boolean canOnePhaseCommit(Snapshot snapshot) throws CommitException {

try {

return super.canOnePhaseCommit(snapshot);

} catch (CommitException e) {

cancelGroupCommitIfNeeded(snapshot.getId());

throw e;

}

@Override

boolean canOnePhaseCommit(Snapshot snapshot) throws CommitException {

try {

return super.canOnePhaseCommit(snapshot);

} catch (CommitException e) {

cancelGroupCommitIfNeeded(snapshot.getId());

throw e; // Re-throw the exception

}

}

gemini-code-assist · 2025-07-09T09:21:43Z

core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerTest.java

+  protected CommitHandler createCommitHandlerWithOnePhaseCommit() {
+    return new CommitHandler(
+        storage, coordinator, tableMetadataManager, parallelExecutor, mutationsGrouper, true, true);
+  }


This method createCommitHandlerWithOnePhaseCommit is duplicated in CommitHandlerTest.java and CommitHandlerWithGroupCommitTest.java. Consider creating this method in a common base class to avoid duplication.

gemini-code-assist · 2025-07-09T09:21:43Z

...rc/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java

  @Test
-  public void canOnePhaseCommit_WhenOnePhaseCommitDisabled_ShouldReturnFalse() {}
-
-  @Disabled("Enabling both one-phase commit and group commit is not supported")
  @Override
-  @Test
-  public void canOnePhaseCommit_WhenValidationRequired_ShouldReturnFalse() {}
+  public void onePhaseCommitRecords_WhenSuccessful_ShouldMutateUsingComposerMutations()
+      throws CommitConflictException, UnknownTransactionStatusException, ExecutionException {
+    super.onePhaseCommitRecords_WhenSuccessful_ShouldMutateUsingComposerMutations();

-  @Disabled("Enabling both one-phase commit and group commit is not supported")
-  @Override
-  @Test
-  public void canOnePhaseCommit_WhenNoWritesAndDeletes_ShouldReturnFalse() {}
+    // Assert
+    verify(groupCommitter).remove(anyId());
+  }


The verify(groupCommitter).remove(anyId()) call is made after calling the super method. It would be more readable if this was done before the super method call, to clearly indicate the order of operations.

Suggested change

@Test

public void canOnePhaseCommit_WhenOnePhaseCommitDisabled_ShouldReturnFalse() {}

@Disabled("Enabling both one-phase commit and group commit is not supported")

@Override

@Test

public void canOnePhaseCommit_WhenValidationRequired_ShouldReturnFalse() {}

public void onePhaseCommitRecords_WhenSuccessful_ShouldMutateUsingComposerMutations()

throws CommitConflictException, UnknownTransactionStatusException, ExecutionException {

super.onePhaseCommitRecords_WhenSuccessful_ShouldMutateUsingComposerMutations();

@Disabled("Enabling both one-phase commit and group commit is not supported")

@Override

@Test

public void canOnePhaseCommit_WhenNoWritesAndDeletes_ShouldReturnFalse() {}

// Assert

verify(groupCommitter).remove(anyId());

}

@Test

@Override

public void onePhaseCommitRecords_WhenSuccessful_ShouldMutateUsingComposerMutations()

throws CommitConflictException, UnknownTransactionStatusException, ExecutionException {

// Assert

verify(groupCommitter).remove(anyId());

super.onePhaseCommitRecords_WhenSuccessful_ShouldMutateUsingComposerMutations();

}

gemini-code-assist · 2025-07-09T09:21:43Z

...rc/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java

  @Test
-  public void canOnePhaseCommit_WhenDeleteWithoutExistingRecord_ShouldReturnFalse() {}
-
-  @Disabled("Enabling both one-phase commit and group commit is not supported")
  @Override
-  @Test
-  public void canOnePhaseCommit_WhenMutationsCanBeGrouped_ShouldReturnTrue() {}
+  public void
+      onePhaseCommitRecords_WhenNoMutationExceptionThrown_ShouldThrowCommitConflictException()
+          throws ExecutionException {
+    super.onePhaseCommitRecords_WhenNoMutationExceptionThrown_ShouldThrowCommitConflictException();

-  @Disabled("Enabling both one-phase commit and group commit is not supported")
-  @Override
-  @Test
-  public void canOnePhaseCommit_WhenMutationsCannotBeGrouped_ShouldReturnFalse() {}
+    // Assert
+    verify(groupCommitter).remove(anyId());
+  }


The verify(groupCommitter).remove(anyId()) call is made after calling the super method. It would be more readable if this was done before the super method call, to clearly indicate the order of operations.

Suggested change

@Test

public void canOnePhaseCommit_WhenDeleteWithoutExistingRecord_ShouldReturnFalse() {}

@Disabled("Enabling both one-phase commit and group commit is not supported")

@Override

@Test

public void canOnePhaseCommit_WhenMutationsCanBeGrouped_ShouldReturnTrue() {}

public void

onePhaseCommitRecords_WhenNoMutationExceptionThrown_ShouldThrowCommitConflictException()

throws ExecutionException {

super.onePhaseCommitRecords_WhenNoMutationExceptionThrown_ShouldThrowCommitConflictException();

@Disabled("Enabling both one-phase commit and group commit is not supported")

@Override

@Test

public void canOnePhaseCommit_WhenMutationsCannotBeGrouped_ShouldReturnFalse() {}

// Assert

verify(groupCommitter).remove(anyId());

}

@Test

@Override

public void

onePhaseCommitRecords_WhenNoMutationExceptionThrown_ShouldThrowCommitConflictException()

throws ExecutionException {

// Assert

verify(groupCommitter).remove(anyId());

super.onePhaseCommitRecords_WhenNoMutationExceptionThrown_ShouldThrowCommitConflictException();

}

gemini-code-assist · 2025-07-09T09:21:44Z

...rc/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java

  @Test
-  public void canOnePhaseCommit_WhenMutationsGrouperThrowsException_ShouldThrowCommitException() {}
+  @Override
+  public void
+      onePhaseCommitRecords_WhenRetriableExecutionExceptionThrown_ShouldThrowCommitConflictException()
+          throws ExecutionException {
+    super
+        .onePhaseCommitRecords_WhenRetriableExecutionExceptionThrown_ShouldThrowCommitConflictException();

-  @Disabled("Enabling both one-phase commit and group commit is not supported")
+    // Assert
+    verify(groupCommitter).remove(anyId());


The verify(groupCommitter).remove(anyId()) call is made after calling the super method. It would be more readable if this was done before the super method call, to clearly indicate the order of operations.

Suggested change

@Test

public void canOnePhaseCommit_WhenMutationsGrouperThrowsException_ShouldThrowCommitException() {}

@Override

public void

onePhaseCommitRecords_WhenRetriableExecutionExceptionThrown_ShouldThrowCommitConflictException()

throws ExecutionException {

super

.onePhaseCommitRecords_WhenRetriableExecutionExceptionThrown_ShouldThrowCommitConflictException();

@Disabled("Enabling both one-phase commit and group commit is not supported")

// Assert

verify(groupCommitter).remove(anyId());

@Test

@Override

public void

onePhaseCommitRecords_WhenRetriableExecutionExceptionThrown_ShouldThrowCommitConflictException()

throws ExecutionException {

// Assert

verify(groupCommitter).remove(anyId());

super

.onePhaseCommitRecords_WhenRetriableExecutionExceptionThrown_ShouldThrowCommitConflictException();

}

gemini-code-assist · 2025-07-09T09:21:44Z

...rc/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java

+
+  @Test
  @Override
+  public void
+      onePhaseCommitRecords_WhenExecutionExceptionThrown_ShouldThrowUnknownTransactionStatusException()
+          throws ExecutionException {
+    super
+        .onePhaseCommitRecords_WhenExecutionExceptionThrown_ShouldThrowUnknownTransactionStatusException();
+
+    // Assert
+    verify(groupCommitter).remove(anyId());


The verify(groupCommitter).remove(anyId()) call is made after calling the super method. It would be more readable if this was done before the super method call, to clearly indicate the order of operations.

@Test @Override public void onePhaseCommitRecords_WhenExecutionExceptionThrown_ShouldThrowUnknownTransactionStatusException() throws ExecutionException { // Assert verify(groupCommitter).remove(anyId()); super .onePhaseCommitRecords_WhenExecutionExceptionThrown_ShouldThrowUnknownTransactionStatusException(); }

gemini-code-assist · 2025-07-09T09:21:44Z

...rc/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java

+  @Test
  @Override
+  public void commit_OnePhaseCommitted_ShouldNotThrowAnyException()
+      throws CommitException, UnknownTransactionStatusException {
+    super.commit_OnePhaseCommitted_ShouldNotThrowAnyException();
+    groupCommitter.remove(anyId());
+  }


The groupCommitter.remove(anyId()) call is made after calling the super method. It would be more readable if this was done before the super method call, to clearly indicate the order of operations.

@Test @Override public void commit_OnePhaseCommitted_ShouldNotThrowAnyException() throws CommitException, UnknownTransactionStatusException { // Assert groupCommitter.remove(anyId()); super.commit_OnePhaseCommitted_ShouldNotThrowAnyException(); }

gemini-code-assist · 2025-07-09T09:21:44Z

...rc/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java

  @Test
+  @Override
  public void
-      commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException() {}
+      commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException()
+          throws CommitException, UnknownTransactionStatusException {
+    super
+        .commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException();
+    groupCommitter.remove(anyId());


The groupCommitter.remove(anyId()) call is made after calling the super method. It would be more readable if this was done before the super method call, to clearly indicate the order of operations.

@Test @Override public void commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException() throws CommitException, UnknownTransactionStatusException { // Assert groupCommitter.remove(anyId()); super .commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException(); }

Fix behavior of group commit with one-phase commit (#2832)

a10354d

feeblefakie assigned brfrn169 Jul 9, 2025

gemini-code-assist bot reviewed Jul 9, 2025

View reviewed changes

brfrn169 merged commit da4a020 into 3 Jul 9, 2025
56 checks passed

brfrn169 deleted the 3-pull-2832 branch July 9, 2025 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Backport to branch(3) : Fix behavior of group commit with one-phase commit #2846

Backport to branch(3) : Fix behavior of group commit with one-phase commit #2846

feeblefakie commented Jul 9, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Jul 9, 2025

Uh oh!

gemini-code-assist bot Jul 9, 2025

Uh oh!

gemini-code-assist bot Jul 9, 2025

Uh oh!

gemini-code-assist bot Jul 9, 2025

Uh oh!

gemini-code-assist bot Jul 9, 2025

Uh oh!

gemini-code-assist bot Jul 9, 2025

Uh oh!

gemini-code-assist bot Jul 9, 2025

Uh oh!

gemini-code-assist bot Jul 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Backport to branch(3) : Fix behavior of group commit with one-phase commit #2846

Backport to branch(3) : Fix behavior of group commit with one-phase commit #2846

Conversation

feeblefakie commented Jul 9, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants