denis-chudov commented on code in PR #7820:
URL: https://github.com/apache/ignite-3/pull/7820#discussion_r3000859803
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImplTest.java:
##########
@@ -950,6 +951,46 @@ void
testScanAfterExceptionalAbortThrowsFinishedWithErrCode() {
}
}
+ @Test
+ void testScanWhileFinishingAfterErrorThrowsFinishedWithErrCode() {
+ InternalTableImpl internalTable = newInternalTable(TABLE_ID, 1);
+
+ InternalTransaction tx = new ReadWriteTransactionImpl(
+ txManager,
+ mock(HybridTimestampTracker.class),
+ TestTransactionIds.newTransactionId(),
+ randomUUID(),
+ false,
+ 1,
+ null
+ );
+
+ UUID txId = tx.id();
+ IllegalStateException failure = new IllegalStateException("boom");
+
+ when(txManager.stateMeta(txId)).thenReturn(new
TxStateMetaFinishing(null, null, false, null, failure, null));
+ tx.rollbackWithExceptionAsync(new
TransactionException(TX_ALREADY_FINISHED_WITH_EXCEPTION_ERR,
+ "Transaction is already finished")).join();
+
+ Publisher<BinaryRow> publisher = internalTable.scan(VALID_PARTITION,
tx, VALID_INDEX_ID, IndexScanCriteria.unbounded());
+
+ CompletableFuture<Void> completed = new CompletableFuture<>();
+
+ publisher.subscribe(new BlackholeSubscriber(completed));
+
+ try {
+ completed.get(10, TimeUnit.SECONDS);
+ fail("Expected TransactionException but scan completed
successfully");
+ } catch (Exception e) {
+ Throwable unwrapped = unwrapCause(e);
+ assertThat("Error should be TransactionException", unwrapped,
is(instanceOf(TransactionException.class)));
+ TransactionException txEx = (TransactionException) unwrapped;
Review Comment:
Could you please insert an empty line before `TransactionException txEx`?
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlApiBaseTest.java:
##########
@@ -736,6 +743,88 @@ public void
runtimeErrorInQueryCausesTransactionToFail(String query) {
"Transaction is already finished due to an error");
}
+ @Test
+ public void
runtimeErrorReturnsSameTransactionErrorBeforeAndAfterRollbackCompletion()
throws Exception {
+ sql("CREATE TABLE tst(id INTEGER PRIMARY KEY, val INTEGER)");
+
+ IgniteSql sql = igniteSql();
+
+ Transaction tx = igniteTx().begin();
+ UUID txId = tx instanceof InternalTransaction ? ((InternalTransaction)
tx).id() : null;
+
+ // Enlist enough operations to make rollback non-trivial.
+ for (int i = 0; i < 100; i++) {
+ execute(tx, sql, "INSERT INTO tst VALUES (?, ?)", i, i);
+ }
+
+ assertThrowsSqlException(
+ Sql.RUNTIME_ERR,
+ "Division by zero",
+ () -> execute(tx, sql, "SELECT val / 0 FROM tst WHERE id = ?",
0)
+ );
+
+ IgniteException[] immediateExceptions = new IgniteException[5];
+ for (int i = 0; i < immediateExceptions.length; i++) {
+ immediateExceptions[i] = (IgniteException) assertThrowsWithCause(
+ () -> executeForRead(sql, tx, "SELECT * FROM tst WHERE id
= ?", 1),
+ IgniteException.class
+ );
+ }
+
+ if (txId != null) {
+ assertTrue(waitForCondition(() -> {
+ TxStateMeta meta = txManager().stateMeta(txId);
+
+ return meta != null && TxState.isFinalState(meta.txState());
+ }, 5_000), "Expected transaction to reach final state");
+ } else {
+ // Client transaction API doesn't expose internal tx id, wait a
bit to compare with a later request.
+ Thread.sleep(500);
+ }
+
+ IgniteException abortedStateException = (IgniteException)
assertThrowsWithCause(
+ () -> executeForRead(sql, tx, "SELECT * FROM tst WHERE id =
?", 1),
+ IgniteException.class
+ );
+
+ assertEquals(Transactions.TX_ALREADY_FINISHED_WITH_EXCEPTION_ERR,
abortedStateException.code());
+ assertTrue(abortedStateException.getMessage().contains("Transaction is
already finished due to an error"));
+
+ for (IgniteException immediateException : immediateExceptions) {
+ assertEquals(abortedStateException.code(),
immediateException.code());
+ assertTrue(immediateException.getMessage().contains("Transaction
is already finished due to an error"));
+ }
+ }
+
+ @Test
+ public void rollbackWithExceptionCauseIsPropagatedToSubsequentSqlRequest()
{
+ sql("CREATE TABLE tst(id INTEGER PRIMARY KEY, val INTEGER)");
+ sql("INSERT INTO tst VALUES (?, ?)", 1, 1);
+
+ Transaction tx = igniteTx().begin();
+
+ assumeTrue(tx instanceof InternalTransaction, "InternalTransaction is
required");
+
+ InternalTransaction internalTx = (InternalTransaction) tx;
+ String rollbackCauseMessage = "rollback-cause-primary-replica-changed";
+ TransactionException rollbackCause = new
TransactionException(REPLICA_MISS_ERR, rollbackCauseMessage);
+
+ await(internalTx.rollbackWithExceptionAsync(rollbackCause));
+
+ IgniteException ex = assertInstanceOf(
+ IgniteException.class,
+ assertThrowsWithCause(
+ () -> executeForRead(igniteSql(), tx, "SELECT * FROM
tst WHERE id = ?", 1),
+ IgniteException.class
+ )
+ );
+
+ assertEquals(Transactions.TX_ALREADY_FINISHED_WITH_EXCEPTION_ERR,
ex.code());
+ assertTrue(ex.getMessage().contains("Transaction is already finished
due to an error"));
+ assertTrue(hasCause(ex, TransactionException.class));
+ assertTrue(hasMessageInChain(ex, rollbackCauseMessage), "Expected
rollback cause message in user-visible exception chain");
Review Comment:
You can use methods like `ExceptionUtils#hasCause(java.lang.Throwable,
java.lang.Class<?>...)` or add new methods to ExceptionUtils if needed.
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlApiBaseTest.java:
##########
@@ -736,6 +743,88 @@ public void
runtimeErrorInQueryCausesTransactionToFail(String query) {
"Transaction is already finished due to an error");
}
+ @Test
+ public void
runtimeErrorReturnsSameTransactionErrorBeforeAndAfterRollbackCompletion()
throws Exception {
+ sql("CREATE TABLE tst(id INTEGER PRIMARY KEY, val INTEGER)");
+
+ IgniteSql sql = igniteSql();
+
+ Transaction tx = igniteTx().begin();
+ UUID txId = tx instanceof InternalTransaction ? ((InternalTransaction)
tx).id() : null;
+
+ // Enlist enough operations to make rollback non-trivial.
+ for (int i = 0; i < 100; i++) {
+ execute(tx, sql, "INSERT INTO tst VALUES (?, ?)", i, i);
+ }
+
+ assertThrowsSqlException(
+ Sql.RUNTIME_ERR,
+ "Division by zero",
+ () -> execute(tx, sql, "SELECT val / 0 FROM tst WHERE id = ?",
0)
+ );
+
+ IgniteException[] immediateExceptions = new IgniteException[5];
+ for (int i = 0; i < immediateExceptions.length; i++) {
+ immediateExceptions[i] = (IgniteException) assertThrowsWithCause(
+ () -> executeForRead(sql, tx, "SELECT * FROM tst WHERE id
= ?", 1),
+ IgniteException.class
+ );
+ }
+
+ if (txId != null) {
+ assertTrue(waitForCondition(() -> {
+ TxStateMeta meta = txManager().stateMeta(txId);
+
+ return meta != null && TxState.isFinalState(meta.txState());
+ }, 5_000), "Expected transaction to reach final state");
+ } else {
+ // Client transaction API doesn't expose internal tx id, wait a
bit to compare with a later request.
+ Thread.sleep(500);
Review Comment:
Looks fragile. You can get transaction id from ClientTransaction, retrieving
it from `ClientLazyTransaction#startedTx` (completed after doing first request
to the server from client)
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxRecoveryEngine.java:
##########
@@ -87,7 +87,6 @@ public CompletableFuture<TransactionMeta> triggerTxRecovery(
// If the transaction state is pending, then the transaction should be
rolled back,
// meaning that the state is changed to aborted and a corresponding
cleanup request
// is sent in a common durable manner to a partition that has
initiated recovery.
- // TODO https://issues.apache.org/jira/browse/IGNITE-27386 the reason
of rollback needs to be explained.
return txManager.finish(
HybridTimestampTracker.emptyTracker(),
// Tx recovery is executed on the commit partition.
Review Comment:
I may have missed that during the previous review, but TX_ROLLBACK_ERR is
incorrect here, it means the rollback failure, not a recovery reason.
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlApiBaseTest.java:
##########
@@ -736,6 +743,88 @@ public void
runtimeErrorInQueryCausesTransactionToFail(String query) {
"Transaction is already finished due to an error");
}
+ @Test
+ public void
runtimeErrorReturnsSameTransactionErrorBeforeAndAfterRollbackCompletion()
throws Exception {
+ sql("CREATE TABLE tst(id INTEGER PRIMARY KEY, val INTEGER)");
+
+ IgniteSql sql = igniteSql();
+
+ Transaction tx = igniteTx().begin();
+ UUID txId = tx instanceof InternalTransaction ? ((InternalTransaction)
tx).id() : null;
+
+ // Enlist enough operations to make rollback non-trivial.
+ for (int i = 0; i < 100; i++) {
+ execute(tx, sql, "INSERT INTO tst VALUES (?, ?)", i, i);
+ }
+
+ assertThrowsSqlException(
+ Sql.RUNTIME_ERR,
+ "Division by zero",
+ () -> execute(tx, sql, "SELECT val / 0 FROM tst WHERE id = ?",
0)
+ );
+
+ IgniteException[] immediateExceptions = new IgniteException[5];
+ for (int i = 0; i < immediateExceptions.length; i++) {
+ immediateExceptions[i] = (IgniteException) assertThrowsWithCause(
+ () -> executeForRead(sql, tx, "SELECT * FROM tst WHERE id
= ?", 1),
+ IgniteException.class
+ );
+ }
+
+ if (txId != null) {
+ assertTrue(waitForCondition(() -> {
Review Comment:
`waitForCondition` is now deprecated, let's use Awaitility.
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImplTest.java:
##########
@@ -950,6 +951,46 @@ void
testScanAfterExceptionalAbortThrowsFinishedWithErrCode() {
}
}
+ @Test
+ void testScanWhileFinishingAfterErrorThrowsFinishedWithErrCode() {
Review Comment:
Could you please add an integration test for the scenario described in the
ticket?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
