From 4524f783edbdb4199784cd3362e4bdcbecfaff51 Mon Sep 17 00:00:00 2001 From: brfrn169 Date: Tue, 13 May 2025 23:46:39 +0900 Subject: [PATCH 1/3] Support begin in read-only mode for JDBC transaction --- .../jdbc/JdbcTransactionIntegrationTest.java | 14 -- .../jdbc/JdbcTransactionManager.java | 82 +++++--- .../jdbc/JdbcTransactionManagerTest.java | 186 ++++++++++++------ 3 files changed, 186 insertions(+), 96 deletions(-) diff --git a/core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionIntegrationTest.java b/core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionIntegrationTest.java index fa67a2d870..37bebaf726 100644 --- a/core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionIntegrationTest.java +++ b/core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionIntegrationTest.java @@ -7,8 +7,6 @@ import java.util.Properties; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.EnumSource; public class JdbcTransactionIntegrationTest extends DistributedTransactionIntegrationTestBase { @@ -44,16 +42,4 @@ public void abort_forOngoingTransaction_ShouldAbortCorrectly() {} @Override @Test public void rollback_forOngoingTransaction_ShouldRollbackCorrectly() {} - - @Disabled("Implement later") - @Override - @Test - public void get_GetGivenForCommittedRecord_InReadOnlyMode_ShouldReturnRecord() {} - - @Disabled("Implement later") - @Override - @ParameterizedTest - @EnumSource(ScanType.class) - public void scanOrGetScanner_ScanGivenForCommittedRecord_InReadOnlyMode_ShouldReturnRecords( - ScanType scanType) {} } diff --git a/core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java b/core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java index b38862f7ba..cf1dc59c64 100644 --- a/core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java +++ b/core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java @@ -18,6 +18,7 @@ import com.scalar.db.api.Upsert; import com.scalar.db.common.AbstractDistributedTransactionManager; import com.scalar.db.common.AbstractTransactionManagerCrudOperableScanner; +import com.scalar.db.common.ReadOnlyDistributedTransaction; import com.scalar.db.common.TableMetadataManager; import com.scalar.db.common.checker.OperationChecker; import com.scalar.db.common.error.CoreError; @@ -36,6 +37,7 @@ import com.scalar.db.storage.jdbc.RdbEngineFactory; import com.scalar.db.storage.jdbc.RdbEngineStrategy; import com.scalar.db.util.ThrowableFunction; +import java.sql.Connection; import java.sql.SQLException; import java.util.List; import java.util.Optional; @@ -90,14 +92,39 @@ public JdbcTransactionManager(DatabaseConfig databaseConfig) { @Override public DistributedTransaction begin() throws TransactionException { String txId = UUID.randomUUID().toString(); - return begin(txId); + return begin(txId, false); } @Override public DistributedTransaction begin(String txId) throws TransactionException { + return begin(txId, false); + } + + @Override + public DistributedTransaction beginReadOnly() throws TransactionException { + String txId = UUID.randomUUID().toString(); + return begin(txId, true); + } + + @Override + public DistributedTransaction beginReadOnly(String txId) throws TransactionException { + return begin(txId, true); + } + + private DistributedTransaction begin(String txId, boolean readOnly) throws TransactionException { try { - JdbcTransaction transaction = - new JdbcTransaction(txId, jdbcService, dataSource.getConnection(), rdbEngine); + Connection connection = dataSource.getConnection(); + + DistributedTransaction transaction; + if (readOnly) { + rdbEngine.setReadOnly(connection, true); + transaction = + new ReadOnlyDistributedTransaction( + new JdbcTransaction(txId, jdbcService, connection, rdbEngine)); + } else { + transaction = new JdbcTransaction(txId, jdbcService, connection, rdbEngine); + } + getNamespace().ifPresent(transaction::withNamespace); getTable().ifPresent(transaction::withTable); return transaction; @@ -109,16 +136,6 @@ public DistributedTransaction begin(String txId) throws TransactionException { } } - @Override - public DistributedTransaction beginReadOnly() { - throw new UnsupportedOperationException("implement later"); - } - - @Override - public DistributedTransaction beginReadOnly(String txId) { - throw new UnsupportedOperationException("implement later"); - } - /** @deprecated As of release 2.4.0. Will be removed in release 4.0.0. */ @SuppressWarnings("InlineMeSuggester") @Deprecated @@ -173,19 +190,19 @@ public DistributedTransaction start( @Override public Optional get(Get get) throws CrudException, UnknownTransactionStatusException { - return executeTransaction(t -> t.get(copyAndSetTargetToIfNot(get))); + return executeTransaction(t -> t.get(copyAndSetTargetToIfNot(get)), true); } @Override public List scan(Scan scan) throws CrudException, UnknownTransactionStatusException { - return executeTransaction(t -> t.scan(copyAndSetTargetToIfNot(scan))); + return executeTransaction(t -> t.scan(copyAndSetTargetToIfNot(scan)), true); } @Override public Scanner getScanner(Scan scan) throws CrudException { DistributedTransaction transaction; try { - transaction = begin(); + transaction = beginReadOnly(); } catch (TransactionNotFoundException e) { throw new CrudConflictException(e.getMessage(), e, e.getTransactionId().orElse(null)); } catch (TransactionException e) { @@ -277,7 +294,8 @@ public void put(Put put) throws CrudException, UnknownTransactionStatusException t -> { t.put(copyAndSetTargetToIfNot(put)); return null; - }); + }, + false); } /** @deprecated As of release 3.13.0. Will be removed in release 5.0.0. */ @@ -288,7 +306,8 @@ public void put(List puts) throws CrudException, UnknownTransactionStatusEx t -> { t.put(copyAndSetTargetToIfNot(puts)); return null; - }); + }, + false); } @Override @@ -297,7 +316,8 @@ public void insert(Insert insert) throws CrudException, UnknownTransactionStatus t -> { t.insert(copyAndSetTargetToIfNot(insert)); return null; - }); + }, + false); } @Override @@ -306,7 +326,8 @@ public void upsert(Upsert upsert) throws CrudException, UnknownTransactionStatus t -> { t.upsert(copyAndSetTargetToIfNot(upsert)); return null; - }); + }, + false); } @Override @@ -315,7 +336,8 @@ public void update(Update update) throws CrudException, UnknownTransactionStatus t -> { t.update(copyAndSetTargetToIfNot(update)); return null; - }); + }, + false); } @Override @@ -324,7 +346,8 @@ public void delete(Delete delete) throws CrudException, UnknownTransactionStatus t -> { t.delete(copyAndSetTargetToIfNot(delete)); return null; - }); + }, + false); } /** @deprecated As of release 3.13.0. Will be removed in release 5.0.0. */ @@ -335,7 +358,8 @@ public void delete(List deletes) throws CrudException, UnknownTransactio t -> { t.delete(copyAndSetTargetToIfNot(deletes)); return null; - }); + }, + false); } @Override @@ -345,15 +369,21 @@ public void mutate(List mutations) t -> { t.mutate(copyAndSetTargetToIfNot(mutations)); return null; - }); + }, + false); } private R executeTransaction( - ThrowableFunction throwableFunction) + ThrowableFunction throwableFunction, + boolean readOnly) throws CrudException, UnknownTransactionStatusException { DistributedTransaction transaction; try { - transaction = begin(); + if (readOnly) { + transaction = beginReadOnly(); + } else { + transaction = begin(); + } } catch (TransactionNotFoundException e) { throw new CrudConflictException(e.getMessage(), e, e.getTransactionId().orElse(null)); } catch (TransactionException e) { diff --git a/core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java b/core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java index 80d0375e6d..5451480e81 100644 --- a/core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java +++ b/core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java @@ -25,6 +25,7 @@ import com.scalar.db.api.Update; import com.scalar.db.api.Upsert; import com.scalar.db.common.ActiveTransactionManagedDistributedTransactionManager; +import com.scalar.db.common.ReadOnlyDistributedTransaction; import com.scalar.db.config.DatabaseConfig; import com.scalar.db.exception.storage.ExecutionException; import com.scalar.db.exception.transaction.AbortException; @@ -131,6 +132,79 @@ public void whenGetOperationsExecutedAndJdbcServiceThrowsSQLException_shouldThro .isInstanceOf(CrudException.class); } + @Test + public void begin_WithoutTxId_ShouldCreateNewTransaction() throws Exception { + // Arrange + Connection connection = mock(Connection.class); + when(dataSource.getConnection()).thenReturn(connection); + + // Act + DistributedTransaction actual = manager.begin(); + + // Assert + verify(dataSource).getConnection(); + assertThat(actual).isInstanceOf(JdbcTransaction.class); + } + + @Test + public void begin_WithTxId_ShouldCreateTransactionWithGivenId() throws Exception { + // Arrange + Connection connection = mock(Connection.class); + when(dataSource.getConnection()).thenReturn(connection); + String txId = "my-tx-id"; + + // Act + DistributedTransaction actual = manager.begin(txId); + + // Assert + verify(dataSource).getConnection(); + assertThat(actual).isInstanceOf(JdbcTransaction.class); + assertThat(actual.getId()).isEqualTo(txId); + } + + @Test + public void beginReadOnly_WithoutTxId_ShouldCreateReadOnlyTransaction() throws Exception { + // Arrange + Connection connection = mock(Connection.class); + when(dataSource.getConnection()).thenReturn(connection); + + // Act + DistributedTransaction actual = manager.beginReadOnly(); + + // Assert + verify(dataSource).getConnection(); + verify(connection).setReadOnly(true); + assertThat(actual).isInstanceOf(ReadOnlyDistributedTransaction.class); + } + + @Test + public void beginReadOnly_WithTxId_ShouldCreateReadOnlyTransactionWithGivenId() throws Exception { + // Arrange + Connection connection = mock(Connection.class); + when(dataSource.getConnection()).thenReturn(connection); + String txId = "my-tx-id"; + + // Act + DistributedTransaction result = manager.beginReadOnly(txId); + + // Assert + verify(dataSource).getConnection(); + verify(connection).setReadOnly(true); + assertThat(result).isInstanceOf(ReadOnlyDistributedTransaction.class); + assertThat(result.getId()).isEqualTo(txId); + } + + @Test + public void begin_ConnectionFailure_ShouldThrowTransactionException() throws Exception { + // Arrange + when(dataSource.getConnection()).thenThrow(new SQLException("Connection failed")); + + // Act & Assert + assertThatThrownBy(() -> manager.begin()) + .isInstanceOf(TransactionException.class) + .hasMessageContaining("Connection failed"); + } + @Test public void get_withConflictError_shouldThrowCrudConflictException() throws SQLException, ExecutionException { @@ -187,7 +261,7 @@ public void getScannerAndScannerOne_ShouldReturnScannerAndReturnProperResult() t DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(); + doReturn(transaction).when(spied).beginReadOnly(); Scan scan = Scan.newBuilder() @@ -217,7 +291,7 @@ public void getScannerAndScannerOne_ShouldReturnScannerAndReturnProperResult() t assertThat(actual.one()).isEmpty(); actual.close(); - verify(spied).begin(); + verify(spied).beginReadOnly(); verify(transaction).commit(); verify(scanner).close(); } @@ -228,7 +302,7 @@ public void getScannerAndScannerAll_ShouldReturnScannerAndReturnProperResults() DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(); + doReturn(transaction).when(spied).beginReadOnly(); Scan scan = Scan.newBuilder() @@ -255,7 +329,7 @@ public void getScannerAndScannerAll_ShouldReturnScannerAndReturnProperResults() assertThat(actual.all()).isEmpty(); actual.close(); - verify(spied).begin(); + verify(spied).beginReadOnly(); verify(transaction).commit(); verify(scanner).close(); } @@ -267,7 +341,7 @@ public void getScannerAndScannerIterator_ShouldReturnScannerAndReturnProperResul DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(); + doReturn(transaction).when(spied).beginReadOnly(); Scan scan = Scan.newBuilder() @@ -302,7 +376,7 @@ public void getScannerAndScannerIterator_ShouldReturnScannerAndReturnProperResul assertThat(iterator.hasNext()).isFalse(); actual.close(); - verify(spied).begin(); + verify(spied).beginReadOnly(); verify(transaction).commit(); verify(scanner).close(); } @@ -313,14 +387,14 @@ public void getScannerAndScannerIterator_ShouldReturnScannerAndReturnProperResul throws TransactionException { // Arrange JdbcTransactionManager spied = spy(manager); - doThrow(TransactionNotFoundException.class).when(spied).begin(); + doThrow(TransactionNotFoundException.class).when(spied).beginReadOnly(); Scan scan = mock(Scan.class); // Act Assert assertThatThrownBy(() -> spied.getScanner(scan)).isInstanceOf(CrudConflictException.class); - verify(spied).begin(); + verify(spied).beginReadOnly(); } @Test @@ -328,14 +402,14 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC throws TransactionException { // Arrange JdbcTransactionManager spied = spy(manager); - doThrow(TransactionException.class).when(spied).begin(); + doThrow(TransactionException.class).when(spied).beginReadOnly(); Scan scan = mock(Scan.class); // Act Assert assertThatThrownBy(() -> spied.getScanner(scan)).isInstanceOf(CrudException.class); - verify(spied).begin(); + verify(spied).beginReadOnly(); } @Test @@ -346,7 +420,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(); + doReturn(transaction).when(spied).beginReadOnly(); Scan scan = Scan.newBuilder() @@ -360,7 +434,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC // Act Assert assertThatThrownBy(() -> spied.getScanner(scan)).isInstanceOf(CrudException.class); - verify(spied).begin(); + verify(spied).beginReadOnly(); verify(transaction).rollback(); } @@ -372,7 +446,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(); + doReturn(transaction).when(spied).beginReadOnly(); Scan scan = Scan.newBuilder() @@ -390,7 +464,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC TransactionManagerCrudOperable.Scanner actual = spied.getScanner(scan); assertThatThrownBy(actual::one).isInstanceOf(CrudException.class); - verify(spied).begin(); + verify(spied).beginReadOnly(); verify(scanner).close(); verify(transaction).rollback(); } @@ -402,7 +476,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC // Arrange DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(); + doReturn(transaction).when(spied).beginReadOnly(); Scan scan = Scan.newBuilder() @@ -420,7 +494,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC TransactionManagerCrudOperable.Scanner actual = spied.getScanner(scan); assertThatThrownBy(actual::all).isInstanceOf(CrudException.class); - verify(spied).begin(); + verify(spied).beginReadOnly(); verify(scanner).close(); verify(transaction).rollback(); } @@ -432,7 +506,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC // Arrange DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(); + doReturn(transaction).when(spied).beginReadOnly(); Scan scan = Scan.newBuilder() @@ -450,7 +524,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC TransactionManagerCrudOperable.Scanner actual = spied.getScanner(scan); assertThatThrownBy(actual::close).isInstanceOf(CrudException.class); - verify(spied).begin(); + verify(spied).beginReadOnly(); verify(scanner).close(); verify(transaction).rollback(); } @@ -463,7 +537,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(); + doReturn(transaction).when(spied).beginReadOnly(); doThrow(CommitConflictException.class).when(transaction).commit(); Scan scan = @@ -480,7 +554,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC TransactionManagerCrudOperable.Scanner actual = spied.getScanner(scan); assertThatThrownBy(actual::close).isInstanceOf(CrudConflictException.class); - verify(spied).begin(); + verify(spied).beginReadOnly(); verify(scanner).close(); verify(transaction).rollback(); } @@ -493,7 +567,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(); + doReturn(transaction).when(spied).beginReadOnly(); doThrow(UnknownTransactionStatusException.class).when(transaction).commit(); Scan scan = @@ -510,7 +584,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC TransactionManagerCrudOperable.Scanner actual = spied.getScanner(scan); assertThatThrownBy(actual::close).isInstanceOf(UnknownTransactionStatusException.class); - verify(spied).begin(); + verify(spied).beginReadOnly(); verify(scanner).close(); } @@ -522,7 +596,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(); + doReturn(transaction).when(spied).beginReadOnly(); doThrow(CommitException.class).when(transaction).commit(); Scan scan = @@ -539,7 +613,7 @@ public void getScanner_TransactionExceptionThrownByTransactionBegin_ShouldThrowC TransactionManagerCrudOperable.Scanner actual = spied.getScanner(scan); assertThatThrownBy(actual::close).isInstanceOf(CrudException.class); - verify(spied).begin(); + verify(spied).beginReadOnly(); verify(scanner).close(); verify(transaction).rollback(); } @@ -994,7 +1068,7 @@ public void get_ShouldGet() throws TransactionException { DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).beginReadOnly(); Get get = Get.newBuilder().namespace("ns").table("tbl").partitionKey(Key.ofInt("pk", 0)).build(); @@ -1006,7 +1080,7 @@ public void get_ShouldGet() throws TransactionException { Optional actual = spied.get(get); // Assert - verify(spied).begin(any()); + verify(spied).beginReadOnly(); verify(transaction).get(get); verify(transaction).commit(); assertThat(actual).isEqualTo(Optional.of(result)); @@ -1018,7 +1092,7 @@ public void get_ShouldGet() throws TransactionException { throws TransactionException { // Arrange JdbcTransactionManager spied = spy(manager); - doThrow(TransactionNotFoundException.class).when(spied).begin(any()); + doThrow(TransactionNotFoundException.class).when(spied).beginReadOnly(); Get get = Get.newBuilder().namespace("ns").table("tbl").partitionKey(Key.ofInt("pk", 0)).build(); @@ -1026,7 +1100,7 @@ public void get_ShouldGet() throws TransactionException { // Act Assert assertThatThrownBy(() -> spied.get(get)).isInstanceOf(CrudConflictException.class); - verify(spied).begin(any()); + verify(spied).beginReadOnly(); } @Test @@ -1034,7 +1108,7 @@ public void get_TransactionExceptionThrownByTransactionBegin_ShouldThrowCrudExce throws TransactionException { // Arrange JdbcTransactionManager spied = spy(manager); - doThrow(TransactionException.class).when(spied).begin(any()); + doThrow(TransactionException.class).when(spied).beginReadOnly(); Get get = Get.newBuilder().namespace("ns").table("tbl").partitionKey(Key.ofInt("pk", 0)).build(); @@ -1042,7 +1116,7 @@ public void get_TransactionExceptionThrownByTransactionBegin_ShouldThrowCrudExce // Act Assert assertThatThrownBy(() -> spied.get(get)).isInstanceOf(CrudException.class); - verify(spied).begin(any()); + verify(spied).beginReadOnly(); } @Test @@ -1052,7 +1126,7 @@ public void get_CrudExceptionThrownByTransactionGet_ShouldThrowCrudException() DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).beginReadOnly(); Get get = Get.newBuilder().namespace("ns").table("tbl").partitionKey(Key.ofInt("pk", 0)).build(); @@ -1061,7 +1135,7 @@ public void get_CrudExceptionThrownByTransactionGet_ShouldThrowCrudException() // Act Assert assertThatThrownBy(() -> spied.get(get)).isInstanceOf(CrudException.class); - verify(spied).begin(any()); + verify(spied).beginReadOnly(); verify(transaction).get(get); verify(transaction).rollback(); } @@ -1074,7 +1148,7 @@ public void get_CrudExceptionThrownByTransactionGet_ShouldThrowCrudException() DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).beginReadOnly(); Get get = Get.newBuilder().namespace("ns").table("tbl").partitionKey(Key.ofInt("pk", 0)).build(); @@ -1083,7 +1157,7 @@ public void get_CrudExceptionThrownByTransactionGet_ShouldThrowCrudException() // Act Assert assertThatThrownBy(() -> spied.get(get)).isInstanceOf(CrudConflictException.class); - verify(spied).begin(any()); + verify(spied).beginReadOnly(); verify(transaction).get(get); verify(transaction).rollback(); } @@ -1096,7 +1170,7 @@ public void get_CrudExceptionThrownByTransactionGet_ShouldThrowCrudException() DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).beginReadOnly(); Get get = Get.newBuilder().namespace("ns").table("tbl").partitionKey(Key.ofInt("pk", 0)).build(); @@ -1105,7 +1179,7 @@ public void get_CrudExceptionThrownByTransactionGet_ShouldThrowCrudException() // Act Assert assertThatThrownBy(() -> spied.get(get)).isInstanceOf(UnknownTransactionStatusException.class); - verify(spied).begin(any()); + verify(spied).beginReadOnly(); verify(transaction).get(get); verify(transaction).commit(); } @@ -1117,7 +1191,7 @@ public void get_CommitExceptionThrownByTransactionCommit_ShouldThrowUCrudExcepti DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).beginReadOnly(); Get get = Get.newBuilder().namespace("ns").table("tbl").partitionKey(Key.ofInt("pk", 0)).build(); @@ -1126,7 +1200,7 @@ public void get_CommitExceptionThrownByTransactionCommit_ShouldThrowUCrudExcepti // Act Assert assertThatThrownBy(() -> spied.get(get)).isInstanceOf(CrudException.class); - verify(spied).begin(any()); + verify(spied).beginReadOnly(); verify(transaction).get(get); verify(transaction).commit(); } @@ -1137,7 +1211,7 @@ public void scan_ShouldScan() throws TransactionException { DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).beginReadOnly(); Scan scan = Scan.newBuilder().namespace("ns").table("tbl").partitionKey(Key.ofInt("pk", 0)).build(); @@ -1150,7 +1224,7 @@ public void scan_ShouldScan() throws TransactionException { List actual = spied.scan(scan); // Assert - verify(spied).begin(any()); + verify(spied).beginReadOnly(); verify(transaction).scan(scan); verify(transaction).commit(); assertThat(actual).isEqualTo(results); @@ -1162,7 +1236,7 @@ public void put_ShouldPut() throws TransactionException { DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).begin(); Put put = Put.newBuilder() @@ -1176,7 +1250,7 @@ public void put_ShouldPut() throws TransactionException { spied.put(put); // Assert - verify(spied).begin(any()); + verify(spied).begin(); verify(transaction).put(put); verify(transaction).commit(); } @@ -1187,7 +1261,7 @@ public void put_MultiplePutsGiven_ShouldPut() throws TransactionException { DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).begin(); List puts = Arrays.asList( @@ -1214,7 +1288,7 @@ public void put_MultiplePutsGiven_ShouldPut() throws TransactionException { spied.put(puts); // Assert - verify(spied).begin(any()); + verify(spied).begin(); verify(transaction).put(puts); verify(transaction).commit(); } @@ -1225,7 +1299,7 @@ public void insert_ShouldInsert() throws TransactionException { DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).begin(); Insert insert = Insert.newBuilder() @@ -1239,7 +1313,7 @@ public void insert_ShouldInsert() throws TransactionException { spied.insert(insert); // Assert - verify(spied).begin(any()); + verify(spied).begin(); verify(transaction).insert(insert); verify(transaction).commit(); } @@ -1250,7 +1324,7 @@ public void upsert_ShouldUpsert() throws TransactionException { DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).begin(); Upsert upsert = Upsert.newBuilder() @@ -1264,7 +1338,7 @@ public void upsert_ShouldUpsert() throws TransactionException { spied.upsert(upsert); // Assert - verify(spied).begin(any()); + verify(spied).begin(); verify(transaction).upsert(upsert); verify(transaction).commit(); } @@ -1275,7 +1349,7 @@ public void update_ShouldUpdate() throws TransactionException { DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).begin(); Update update = Update.newBuilder() @@ -1289,7 +1363,7 @@ public void update_ShouldUpdate() throws TransactionException { spied.update(update); // Assert - verify(spied).begin(any()); + verify(spied).begin(); verify(transaction).update(update); verify(transaction).commit(); } @@ -1300,7 +1374,7 @@ public void delete_ShouldDelete() throws TransactionException { DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).begin(); Delete delete = Delete.newBuilder().namespace("ns").table("tbl").partitionKey(Key.ofInt("pk", 0)).build(); @@ -1309,7 +1383,7 @@ public void delete_ShouldDelete() throws TransactionException { spied.delete(delete); // Assert - verify(spied).begin(any()); + verify(spied).begin(); verify(transaction).delete(delete); verify(transaction).commit(); } @@ -1320,7 +1394,7 @@ public void delete_MultipleDeletesGiven_ShouldDelete() throws TransactionExcepti DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).begin(); List deletes = Arrays.asList( @@ -1344,7 +1418,7 @@ public void delete_MultipleDeletesGiven_ShouldDelete() throws TransactionExcepti spied.delete(deletes); // Assert - verify(spied).begin(any()); + verify(spied).begin(); verify(transaction).delete(deletes); verify(transaction).commit(); } @@ -1355,7 +1429,7 @@ public void mutate_ShouldMutate() throws TransactionException { DistributedTransaction transaction = mock(DistributedTransaction.class); JdbcTransactionManager spied = spy(manager); - doReturn(transaction).when(spied).begin(any()); + doReturn(transaction).when(spied).begin(); List mutations = Arrays.asList( @@ -1393,7 +1467,7 @@ public void mutate_ShouldMutate() throws TransactionException { spied.mutate(mutations); // Assert - verify(spied).begin(any()); + verify(spied).begin(); verify(transaction).mutate(mutations); verify(transaction).commit(); } From 60c000f53be53a374af60964faf6428d61709285 Mon Sep 17 00:00:00 2001 From: brfrn169 Date: Fri, 6 Jun 2025 20:30:14 +0900 Subject: [PATCH 2/3] Rename setReadOnly() to setConnectionToReadOnly() --- .../java/com/scalar/db/storage/jdbc/JdbcAdmin.java | 10 +++++----- .../java/com/scalar/db/storage/jdbc/JdbcDatabase.java | 4 ++-- .../com/scalar/db/storage/jdbc/RdbEngineSqlite.java | 2 +- .../com/scalar/db/storage/jdbc/RdbEngineStrategy.java | 3 ++- .../db/transaction/jdbc/JdbcTransactionManager.java | 2 +- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java index e084b63ae2..a9e34787b7 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java @@ -438,7 +438,7 @@ public TableMetadata getTableMetadata(String namespace, String table) throws Exe boolean tableExists = false; try (Connection connection = dataSource.getConnection()) { - rdbEngine.setReadOnly(connection, true); + rdbEngine.setConnectionToReadOnly(connection, true); try (PreparedStatement preparedStatement = connection.prepareStatement(getSelectColumnsStatement())) { @@ -510,7 +510,7 @@ public TableMetadata getImportTableMetadata( } try (Connection connection = dataSource.getConnection()) { - rdbEngine.setReadOnly(connection, true); + rdbEngine.setConnectionToReadOnly(connection, true); String catalogName = rdbEngine.getCatalogName(namespace); String schemaName = rdbEngine.getSchemaName(namespace); @@ -608,7 +608,7 @@ public Set getNamespaceTableNames(String namespace) throws ExecutionExce + enclose(METADATA_COL_FULL_TABLE_NAME) + " LIKE ?"; try (Connection connection = dataSource.getConnection()) { - rdbEngine.setReadOnly(connection, true); + rdbEngine.setConnectionToReadOnly(connection, true); try (PreparedStatement preparedStatement = connection.prepareStatement(selectTablesOfNamespaceStatement)) { @@ -644,7 +644,7 @@ public boolean namespaceExists(String namespace) throws ExecutionException { + enclose(NAMESPACE_COL_NAMESPACE_NAME) + " = ?"; try (Connection connection = dataSource.getConnection()) { - rdbEngine.setReadOnly(connection, true); + rdbEngine.setConnectionToReadOnly(connection, true); try (PreparedStatement statement = connection.prepareStatement(selectQuery)) { statement.setString(1, namespace); @@ -992,7 +992,7 @@ private String encloseFullTableName(String schema, String table) { @Override public Set getNamespaceNames() throws ExecutionException { try (Connection connection = dataSource.getConnection()) { - rdbEngine.setReadOnly(connection, true); + rdbEngine.setConnectionToReadOnly(connection, true); String selectQuery = "SELECT * FROM " + encloseFullTableName(metadataSchema, NAMESPACES_TABLE); diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java index 7a17ea1a34..c6c6af21d7 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java @@ -82,7 +82,7 @@ public Optional get(Get get) throws ExecutionException { Connection connection = null; try { connection = dataSource.getConnection(); - rdbEngine.setReadOnly(connection, true); + rdbEngine.setConnectionToReadOnly(connection, true); return jdbcService.get(get, connection); } catch (SQLException e) { throw new ExecutionException( @@ -98,7 +98,7 @@ public Scanner scan(Scan scan) throws ExecutionException { Connection connection = null; try { connection = dataSource.getConnection(); - rdbEngine.setReadOnly(connection, true); + rdbEngine.setConnectionToReadOnly(connection, true); return jdbcService.getScanner(scan, connection); } catch (SQLException e) { close(connection); diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlite.java b/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlite.java index c2e39ec827..0501d9a22e 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlite.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlite.java @@ -340,7 +340,7 @@ public RdbEngineTimeTypeStrategy getTimeTypeStrategy( } @Override - public void setReadOnly(Connection connection, boolean readOnly) { + public void setConnectionToReadOnly(Connection connection, boolean readOnly) { // Do nothing. SQLite does not support read-only mode. } } diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java b/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java index 9195dd7975..583cf016ee 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java @@ -230,7 +230,8 @@ default void throwIfDuplicatedIndexWarning(SQLWarning warning) throws SQLExcepti // Do nothing } - default void setReadOnly(Connection connection, boolean readOnly) throws SQLException { + default void setConnectionToReadOnly(Connection connection, boolean readOnly) + throws SQLException { connection.setReadOnly(readOnly); } } diff --git a/core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java b/core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java index cf1dc59c64..0f334227af 100644 --- a/core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java +++ b/core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java @@ -117,7 +117,7 @@ private DistributedTransaction begin(String txId, boolean readOnly) throws Trans DistributedTransaction transaction; if (readOnly) { - rdbEngine.setReadOnly(connection, true); + rdbEngine.setConnectionToReadOnly(connection, true); transaction = new ReadOnlyDistributedTransaction( new JdbcTransaction(txId, jdbcService, connection, rdbEngine)); From 3a2e33c15fa3a0ac00e4736ac9d2534b3ffc3f39 Mon Sep 17 00:00:00 2001 From: brfrn169 Date: Fri, 6 Jun 2025 23:07:23 +0900 Subject: [PATCH 3/3] Fix --- .../transaction/jdbc/JdbcTransactionManagerTest.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java b/core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java index 5451480e81..8a34348e14 100644 --- a/core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java +++ b/core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java @@ -195,14 +195,12 @@ public void beginReadOnly_WithTxId_ShouldCreateReadOnlyTransactionWithGivenId() } @Test - public void begin_ConnectionFailure_ShouldThrowTransactionException() throws Exception { + public void begin_SQLExceptionThrown_ShouldThrowTransactionException() throws Exception { // Arrange - when(dataSource.getConnection()).thenThrow(new SQLException("Connection failed")); + when(dataSource.getConnection()).thenThrow(SQLException.class); - // Act & Assert - assertThatThrownBy(() -> manager.begin()) - .isInstanceOf(TransactionException.class) - .hasMessageContaining("Connection failed"); + // Act Assert + assertThatThrownBy(() -> manager.begin()).isInstanceOf(TransactionException.class); } @Test