From d6eb0cd3c8298eaf11eec95266e313721e1ef259 Mon Sep 17 00:00:00 2001 From: Mitsunori Komatsu Date: Mon, 17 Jun 2024 12:02:41 +0900 Subject: [PATCH] Delay throwing an exception for the combination of two-phase commit interface and the group commit feature to when `begin()` is called (#1903) --- .../TwoPhaseConsensusCommitManager.java | 8 +-- .../TwoPhaseConsensusCommitManagerTest.java | 65 +++++++++++++------ 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java b/core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java index 5504382b8a..864ecc7689 100644 --- a/core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java +++ b/core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java @@ -44,7 +44,6 @@ public TwoPhaseConsensusCommitManager( this.storage = storage; this.admin = admin; config = new ConsensusCommitConfig(databaseConfig); - throwIfGroupCommitIsEnabled(config); tableMetadataManager = new TransactionTableMetadataManager( admin, databaseConfig.getMetadataCacheExpirationTimeSecs()); @@ -61,9 +60,7 @@ public TwoPhaseConsensusCommitManager(DatabaseConfig databaseConfig) { StorageFactory storageFactory = StorageFactory.create(databaseConfig.getProperties()); storage = storageFactory.getStorage(); admin = storageFactory.getStorageAdmin(); - config = new ConsensusCommitConfig(databaseConfig); - throwIfGroupCommitIsEnabled(config); tableMetadataManager = new TransactionTableMetadataManager( admin, databaseConfig.getMetadataCacheExpirationTimeSecs()); @@ -90,7 +87,6 @@ public TwoPhaseConsensusCommitManager(DatabaseConfig databaseConfig) { this.storage = storage; this.admin = admin; this.config = config; - throwIfGroupCommitIsEnabled(config); tableMetadataManager = new TransactionTableMetadataManager( admin, databaseConfig.getMetadataCacheExpirationTimeSecs()); @@ -102,7 +98,7 @@ public TwoPhaseConsensusCommitManager(DatabaseConfig databaseConfig) { mutationOperationChecker = new ConsensusCommitMutationOperationChecker(tableMetadataManager); } - private void throwIfGroupCommitIsEnabled(ConsensusCommitConfig config) { + private void throwIfGroupCommitIsEnabled() { if (CoordinatorGroupCommitter.isEnabled(config)) { throw new IllegalArgumentException( CoreError.CONSENSUS_COMMIT_GROUP_COMMIT_WITH_TWO_PHASE_COMMIT_INTERFACE_NOT_ALLOWED @@ -132,6 +128,7 @@ TwoPhaseCommitTransaction begin(Isolation isolation, SerializableStrategy strate @VisibleForTesting TwoPhaseCommitTransaction begin(String txId, Isolation isolation, SerializableStrategy strategy) throws TransactionException { + throwIfGroupCommitIsEnabled(); return createNewTransaction(txId, isolation, strategy); } @@ -144,6 +141,7 @@ public TwoPhaseCommitTransaction join(String txId) throws TransactionException { @VisibleForTesting TwoPhaseCommitTransaction join(String txId, Isolation isolation, SerializableStrategy strategy) throws TransactionException { + throwIfGroupCommitIsEnabled(); // If the transaction associated with the specified transaction ID is active, resume it if (isTransactionActive(txId)) { return resume(txId); diff --git a/core/src/test/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManagerTest.java b/core/src/test/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManagerTest.java index d1466bd6e0..7ea78159eb 100644 --- a/core/src/test/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManagerTest.java +++ b/core/src/test/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManagerTest.java @@ -80,26 +80,6 @@ public void setUp() throws Exception { commit); } - @Test - public void constructor_GroupCommitEnabledGiven_ShouldThrowException() { - // Arrange - when(config.isCoordinatorGroupCommitEnabled()).thenReturn(true); - - // Act Assert - assertThatThrownBy( - () -> - new TwoPhaseConsensusCommitManager( - storage, - admin, - config, - databaseConfig, - coordinator, - parallelExecutor, - recovery, - commit)) - .isInstanceOf(IllegalArgumentException.class); - } - @Test public void begin_NoArgumentGiven_ReturnWithSomeTxIdAndSnapshotIsolation() throws TransactionException { @@ -168,6 +148,24 @@ public void begin_CalledTwiceWithSameTxId_ThrowTransactionException() assertThatThrownBy(() -> manager.begin(ANY_TX_ID)).isInstanceOf(TransactionException.class); } + @Test + public void begin_NoArgumentGiven_WithGroupCommitEnabled_ShouldThrowException() { + // Arrange + when(config.isCoordinatorGroupCommitEnabled()).thenReturn(true); + + // Act Assert + assertThatThrownBy(() -> manager.begin()).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void begin_TxIdGiven_WithGroupCommitEnabled_ShouldThrowException() { + // Arrange + when(config.isCoordinatorGroupCommitEnabled()).thenReturn(true); + + // Act Assert + assertThatThrownBy(() -> manager.begin(ANY_TX_ID)).isInstanceOf(IllegalArgumentException.class); + } + @Test public void start_NoArgumentGiven_ReturnWithSomeTxIdAndSnapshotIsolation() throws TransactionException { @@ -236,6 +234,24 @@ public void start_CalledTwiceWithSameTxId_ThrowTransactionException() assertThatThrownBy(() -> manager.start(ANY_TX_ID)).isInstanceOf(TransactionException.class); } + @Test + public void start_NoArgumentGiven_WithGroupCommitEnabled_ShouldThrowException() { + // Arrange + when(config.isCoordinatorGroupCommitEnabled()).thenReturn(true); + + // Act Assert + assertThatThrownBy(() -> manager.start()).isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void start_TxIdGiven_WithGroupCommitEnabled_ShouldThrowException() { + // Arrange + when(config.isCoordinatorGroupCommitEnabled()).thenReturn(true); + + // Act Assert + assertThatThrownBy(() -> manager.start(ANY_TX_ID)).isInstanceOf(IllegalArgumentException.class); + } + @Test public void join_TxIdGiven_ReturnWithSpecifiedTxIdAndSnapshotIsolation() throws TransactionException { @@ -265,6 +281,15 @@ public void join_CalledAfterJoinWithSameTxId_ReturnSameTransactionObject() assertThat(transaction1).isEqualTo(transaction2); } + @Test + public void join_TxIdGiven_WithGroupCommitEnabled_ShouldThrowException() { + // Arrange + when(config.isCoordinatorGroupCommitEnabled()).thenReturn(true); + + // Act Assert + assertThatThrownBy(() -> manager.join(ANY_TX_ID)).isInstanceOf(IllegalArgumentException.class); + } + @Test public void resume_CalledWithBegin_ReturnSameTransactionObject() throws TransactionException { // Arrange