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

Author: brfrn169

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

@@ -57,6 +57,23 @@ public void commit(Snapshot snapshot, boolean readOnly)
     super.commit(snapshot, readOnly);
   }
 
+  @Override
+  boolean canOnePhaseCommit(Snapshot snapshot) throws CommitException {
+    try {
+      return super.canOnePhaseCommit(snapshot);
+    } catch (CommitException e) {
+      cancelGroupCommitIfNeeded(snapshot.getId());
+      throw e;
+    }
+  }
+
+  @Override
+  void onePhaseCommitRecords(Snapshot snapshot)
+      throws CommitConflictException, UnknownTransactionStatusException {
+    cancelGroupCommitIfNeeded(snapshot.getId());
+    super.onePhaseCommitRecords(snapshot);
+  }
+
   @Override
   protected void onFailureBeforeCommit(Snapshot snapshot) {
     cancelGroupCommitIfNeeded(snapshot.getId());

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

@@ -90,6 +90,11 @@ protected CommitHandler createCommitHandler(boolean coordinatorWriteOmissionOnRe
         false);
   }
 
+  protected CommitHandler createCommitHandlerWithOnePhaseCommit() {
+    return new CommitHandler(
+        storage, coordinator, tableMetadataManager, parallelExecutor, mutationsGrouper, true, true);
+  }
+
   @BeforeEach
   void setUp() throws Exception {
     MockitoAnnotations.openMocks(this).close();
@@ -1068,8 +1073,9 @@ public void canOnePhaseCommit_WhenMutationsCannotBeGrouped_ShouldReturnFalse() t
   }
 
   @Test
-  public void canOnePhaseCommit_WhenMutationsGrouperThrowsException_ShouldThrowCommitException()
-      throws ExecutionException {
+  public void
+      canOnePhaseCommit_WhenMutationsGrouperThrowsExecutionException_ShouldThrowCommitException()
+          throws ExecutionException {
     // Arrange
     CommitHandler handler = createCommitHandlerWithOnePhaseCommit();
     Snapshot snapshot = prepareSnapshot();
@@ -1180,11 +1186,6 @@ public void commit_OnePhaseCommitted_ShouldNotThrowAnyException()
     verify(handler).onFailureBeforeCommit(snapshot);
   }
 
-  private CommitHandler createCommitHandlerWithOnePhaseCommit() {
-    return new CommitHandler(
-        storage, coordinator, tableMetadataManager, parallelExecutor, mutationsGrouper, true, true);
-  }
-
   protected void doThrowExceptionWhenCoordinatorPutState(
       TransactionState targetState, Class<? extends Exception> exceptionClass)
       throws CoordinatorException {

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

@@ -15,14 +15,14 @@
 
 import com.scalar.db.api.TransactionState;
 import com.scalar.db.exception.storage.ExecutionException;
+import com.scalar.db.exception.transaction.CommitConflictException;
 import com.scalar.db.exception.transaction.CommitException;
 import com.scalar.db.exception.transaction.UnknownTransactionStatusException;
 import com.scalar.db.exception.transaction.ValidationConflictException;
 import com.scalar.db.transaction.consensuscommit.CoordinatorGroupCommitter.CoordinatorGroupCommitKeyManipulator;
 import com.scalar.db.util.groupcommit.GroupCommitConfig;
 import java.util.List;
 import java.util.UUID;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
@@ -75,6 +75,20 @@ protected CommitHandler createCommitHandler(boolean coordinatorWriteOmissionOnRe
         groupCommitter);
   }
 
+  @Override
+  protected CommitHandler createCommitHandlerWithOnePhaseCommit() {
+    createGroupCommitterIfNotExists();
+    return new CommitHandlerWithGroupCommit(
+        storage,
+        coordinator,
+        tableMetadataManager,
+        parallelExecutor,
+        mutationsGrouper,
+        true,
+        true,
+        groupCommitter);
+  }
+
   private String anyGroupCommitParentId() {
     return parentKey;
   }
@@ -199,72 +213,121 @@ public void commit_NoReadsInSnapshot_ShouldNotValidateRecords(boolean withSnapsh
     verify(groupCommitter, never()).remove(anyId());
   }
 
-  @Disabled("Enabling both one-phase commit and group commit is not supported")
-  @Override
   @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());
+  }
 
-  @Disabled("Enabling both one-phase commit and group commit is not supported")
-  @Override
   @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());
+  }
 
-  @Disabled("Enabling both one-phase commit and group commit is not supported")
-  @Override
   @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_WhenExecutionExceptionThrown_ShouldThrowUnknownTransactionStatusException()
+          throws ExecutionException {
+    super
+        .onePhaseCommitRecords_WhenExecutionExceptionThrown_ShouldThrowUnknownTransactionStatusException();
+
+    // Assert
+    verify(groupCommitter).remove(anyId());
+  }
+
   @Test
-  public void onePhaseCommitRecords_WhenSuccessful_ShouldMutateUsingComposerMutations() {}
+  @Override
+  public void canOnePhaseCommit_WhenOnePhaseCommitDisabled_ShouldReturnFalse() throws Exception {
+    super.canOnePhaseCommit_WhenOnePhaseCommitDisabled_ShouldReturnFalse();
+    groupCommitter.remove(anyId());
+  }
 
-  @Disabled("Enabling both one-phase commit and group commit is not supported")
+  @Test
   @Override
+  public void canOnePhaseCommit_WhenValidationRequired_ShouldReturnFalse() throws Exception {
+    super.canOnePhaseCommit_WhenValidationRequired_ShouldReturnFalse();
+    groupCommitter.remove(anyId());
+  }
+
   @Test
-  public void
-      onePhaseCommitRecords_WhenNoMutationExceptionThrown_ShouldThrowCommitConflictException() {}
+  @Override
+  public void canOnePhaseCommit_WhenNoWritesAndDeletes_ShouldReturnFalse() throws Exception {
+    super.canOnePhaseCommit_WhenNoWritesAndDeletes_ShouldReturnFalse();
+    groupCommitter.remove(anyId());
+  }
 
-  @Disabled("Enabling both one-phase commit and group commit is not supported")
+  @Test
   @Override
+  public void canOnePhaseCommit_WhenDeleteWithoutExistingRecord_ShouldReturnFalse()
+      throws Exception {
+    super.canOnePhaseCommit_WhenDeleteWithoutExistingRecord_ShouldReturnFalse();
+    groupCommitter.remove(anyId());
+  }
+
   @Test
-  public void
-      onePhaseCommitRecords_WhenRetriableExecutionExceptionThrown_ShouldThrowCommitConflictException() {}
+  @Override
+  public void canOnePhaseCommit_WhenMutationsCanBeGrouped_ShouldReturnTrue() throws Exception {
+    super.canOnePhaseCommit_WhenMutationsCanBeGrouped_ShouldReturnTrue();
+    groupCommitter.remove(anyId());
+  }
 
-  @Disabled("Enabling both one-phase commit and group commit is not supported")
+  @Test
   @Override
+  public void canOnePhaseCommit_WhenMutationsCannotBeGrouped_ShouldReturnFalse() throws Exception {
+    super.canOnePhaseCommit_WhenMutationsCannotBeGrouped_ShouldReturnFalse();
+    groupCommitter.remove(anyId());
+  }
+
   @Test
+  @Override
   public void
-      onePhaseCommitRecords_WhenExecutionExceptionThrown_ShouldThrowUnknownTransactionStatusException() {}
+      canOnePhaseCommit_WhenMutationsGrouperThrowsExecutionException_ShouldThrowCommitException()
+          throws ExecutionException {
+    super
+        .canOnePhaseCommit_WhenMutationsGrouperThrowsExecutionException_ShouldThrowCommitException();
 
-  @Disabled("Enabling both one-phase commit and group commit is not supported")
-  @Override
-  @Test
-  public void commit_OnePhaseCommitted_ShouldNotThrowAnyException() {}
+    // Assert
+    verify(groupCommitter).remove(anyId());
+  }
 
-  @Disabled("Enabling both one-phase commit and group commit is not supported")
+  @Test
   @Override
+  public void commit_OnePhaseCommitted_ShouldNotThrowAnyException()
+      throws CommitException, UnknownTransactionStatusException {
+    super.commit_OnePhaseCommitted_ShouldNotThrowAnyException();
+    groupCommitter.remove(anyId());
+  }
+
   @Test
+  @Override
   public void
-      commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException() {}
+      commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException()
+          throws CommitException, UnknownTransactionStatusException {
+    super
+        .commit_OnePhaseCommitted_UnknownTransactionStatusExceptionThrown_ShouldThrowUnknownTransactionStatusException();
+    groupCommitter.remove(anyId());
+  }
 }