You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "denis-chudov (via GitHub)" <gi...@apache.org> on 2024/01/25 16:39:43 UTC
[PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
denis-chudov opened a new pull request, #3092:
URL: https://github.com/apache/ignite-3/pull/3092
https://issues.apache.org/jira/browse/IGNITE-20894
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1478040795
##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/IgniteTestUtils.java:
##########
@@ -322,6 +326,35 @@ public static Throwable assertThrowsWithCause(
throw new AssertionError("Exception has not been thrown.");
}
+ /**
+ * Checks the given {@link IgniteException} that it is the instance of the correct class, has the correct code and message.
+ * If the exception is incorrect, {@link AssertionError} is thrown and the incorrect exception is added to suppressed exceptions.
+ *
+ * @param expectedClass Expected class of the exception.
+ * @param expectedErrorCode Expected error code of the {@link IgniteException}.
+ * @param expectedMessageSubstring A substring that must be contained in the error message of the exception.
+ * @param exception Exception to check.
+ */
+ public static void assertExceptionIsExpected(
Review Comment:
It's not "with cause" because it checks the exact type of the exception, and it seems to be more natural because if we assume that the exception has some error code, it should be the exception of some desired type and we don't have to check it's cause.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1475975221
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -1944,7 +1944,7 @@ private <T> CompletableFuture<T> appendTxCommand(UUID txId, RequestType cmdType,
});
if (cleanupReadyFut.isCompletedExceptionally()) {
- return failedFuture(new TransactionException(TX_FAILED_READ_WRITE_OPERATION_ERR, "Transaction is already finished."));
+ return failedFuture(new TransactionException(TX_ALREADY_FINISHED_ERR, "Transaction is already finished."));
Review Comment:
yes, see here
```
if (txStateMeta == null || isFinalState(txStateMeta.txState())) {
cleanupReadyFut.completeExceptionally(new Exception());
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1476021017
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -1944,7 +1944,7 @@ private <T> CompletableFuture<T> appendTxCommand(UUID txId, RequestType cmdType,
});
if (cleanupReadyFut.isCompletedExceptionally()) {
- return failedFuture(new TransactionException(TX_FAILED_READ_WRITE_OPERATION_ERR, "Transaction is already finished."));
+ return failedFuture(new TransactionException(TX_ALREADY_FINISHED_ERR, "Transaction is already finished."));
Review Comment:
Perfect!
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1472794691
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2157,150 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
Review Comment:
There is an upsert to this tx above, but we be test the empty txs as well
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1475971403
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2178,13 +2340,13 @@ protected void testTransactionAlreadyFinished(boolean commit, boolean checkLocks
assertTrue(ex.getMessage().contains("Transaction is already finished."));
ex = assertThrows(TransactionException.class, () -> accountsRv.delete(tx, makeKey(1)));
- assertTrue(ex.getMessage().contains("Failed to enlist"));
+ assertTrue(ex.getMessage().contains("Transaction is already finished"));
Review Comment:
Should we use new utility method here that will also check code?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1478032071
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -1944,7 +1944,7 @@ private <T> CompletableFuture<T> appendTxCommand(UUID txId, RequestType cmdType,
});
if (cleanupReadyFut.isCompletedExceptionally()) {
- return failedFuture(new TransactionException(TX_FAILED_READ_WRITE_OPERATION_ERR, "Transaction is already finished."));
+ return failedFuture(new TransactionException(TX_ALREADY_FINISHED_ERR, "Transaction is already finished."));
Review Comment:
Still perfect!
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1478058494
##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/IgniteTestUtils.java:
##########
@@ -322,6 +326,35 @@ public static Throwable assertThrowsWithCause(
throw new AssertionError("Exception has not been thrown.");
}
+ /**
+ * Checks the given {@link IgniteException} that it is the instance of the correct class, has the correct code and message.
+ * If the exception is incorrect, {@link AssertionError} is thrown and the incorrect exception is added to suppressed exceptions.
+ *
+ * @param expectedClass Expected class of the exception.
+ * @param expectedErrorCode Expected error code of the {@link IgniteException}.
+ * @param expectedMessageSubstring A substring that must be contained in the error message of the exception.
+ * @param exception Exception to check.
+ */
+ public static void assertExceptionIsExpected(
Review Comment:
Ok, you are right.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1477884853
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -1944,7 +1944,7 @@ private <T> CompletableFuture<T> appendTxCommand(UUID txId, RequestType cmdType,
});
if (cleanupReadyFut.isCompletedExceptionally()) {
- return failedFuture(new TransactionException(TX_FAILED_READ_WRITE_OPERATION_ERR, "Transaction is already finished."));
+ return failedFuture(new TransactionException(TX_ALREADY_FINISHED_ERR, "Transaction is already finished."));
Review Comment:
yes, see this
```
if (txStateMeta == null || isFinalState(txStateMeta.txState())) {
cleanupReadyFut.completeExceptionally(new Exception());
```
##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/IgniteTestUtils.java:
##########
@@ -322,6 +326,35 @@ public static Throwable assertThrowsWithCause(
throw new AssertionError("Exception has not been thrown.");
}
+ /**
+ * Checks the given {@link IgniteException} that it is the instance of the correct class, has the correct code and message.
+ * If the exception is incorrect, {@link AssertionError} is thrown and the incorrect exception is added to suppressed exceptions.
+ *
+ * @param expectedClass Expected class of the exception.
+ * @param expectedErrorCode Expected error code of the {@link IgniteException}.
+ * @param expectedMessageSubstring A substring that must be contained in the error message of the exception.
+ * @param exception Exception to check.
+ */
+ public static void assertExceptionIsExpected(
Review Comment:
done
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1477971660
##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/IgniteTestUtils.java:
##########
@@ -282,6 +285,39 @@ public static Throwable assertThrows(
return throwable;
}
+ /**
+ * Checks whether runnable throws the correct {@link IgniteException}, which is itself of a specified class.
+ *
+ * @param expectedClass Expected exception class.
+ * @param expectedErrorCode Expected error code of the {@link IgniteException}.
+ * @param run Runnable to check.
+ * @param errorMessageFragment Fragment of the error text in the expected exception, {@code null} if not to be checked.
+ * @return Thrown throwable.
+ */
+ public static Throwable assertThrowsWithCode(
Review Comment:
Why it's not assertThrowsWith**Cause**?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1478032420
##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/IgniteTestUtils.java:
##########
@@ -322,6 +326,35 @@ public static Throwable assertThrowsWithCause(
throw new AssertionError("Exception has not been thrown.");
}
+ /**
+ * Checks the given {@link IgniteException} that it is the instance of the correct class, has the correct code and message.
+ * If the exception is incorrect, {@link AssertionError} is thrown and the incorrect exception is added to suppressed exceptions.
+ *
+ * @param expectedClass Expected class of the exception.
+ * @param expectedErrorCode Expected error code of the {@link IgniteException}.
+ * @param expectedMessageSubstring A substring that must be contained in the error message of the exception.
+ * @param exception Exception to check.
+ */
+ public static void assertExceptionIsExpected(
Review Comment:
Why it's not assertThrowsWith**Cause**?
What I mean here is that I'd expect 3 overloaded versions where less specific ones will call generalized with null as code or message just like current assertThrowsWithCause do
```
public static Throwable assertThrowsWithCause(
RunnableX run,
Class<? extends Throwable> cls
) {
return assertThrowsWithCause(run, cls, null);
}
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1475879081
##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/IgniteTestUtils.java:
##########
@@ -322,6 +326,35 @@ public static Throwable assertThrowsWithCause(
throw new AssertionError("Exception has not been thrown.");
}
+ /**
+ * Checks the given {@link IgniteException} that it is the instance of the correct class, has the correct code and message.
+ * If the exception is incorrect, {@link AssertionError} is thrown and the incorrect exception is added to suppressed exceptions.
+ *
+ * @param expectedClass Expected class of the exception.
+ * @param expectedErrorCode Expected error code of the {@link IgniteException}.
+ * @param expectedMessageSubstring A substring that must be contained in the error message of the exception.
+ * @param exception Exception to check.
+ */
+ public static void assertExceptionIsExpected(
Review Comment:
I'd expected that it'll be an overloaded version of `assertThrowsWithCause` with code parameter in addition to cls and msg that will use extended `hasCause` with extra code param
```
public static boolean hasCause(
Throwable t,
Class<?> cls,
@Nullable Integer errorCode
@Nullable String messageFragment
) {
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1474654844
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2157,150 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
+
+ finishLatch.countDown();
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ assertThat(futFinishes, willSucceedFast());
+ assertThat(futEnlists, willSucceedFast());
+
+ assertEquals(threadNum, enlistExceptions.size());
+
+ for (var e : enlistExceptions) {
+ assertInstanceOf(TransactionException.class, e);
+ }
+
+ assertTrue(CollectionUtils.nullOrEmpty(txManager(accounts).lockManager().locks(txId)));
+ }
+
+ /**
+ * Test trying to finish a read only tx in multiple threads simultaneously.
+ */
+ @RepeatedTest(10)
+ public void testReadOnlyTransactionMultiThreadedFinish() {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin(new TransactionOptions().readOnly(true));
+
+ rv.get(tx, makeKey(1));
+
+ int threadNum = Runtime.getRuntime().availableProcessors();
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, -1);
Review Comment:
Yes. It would be nice.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1474687004
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -520,7 +520,7 @@ private <R> CompletableFuture<R> trackingInvoke(
// Track only write requests from explicit transactions.
if (!txManager.addInflight(tx.id())) {
return failedFuture(
- new TransactionException(TX_UNEXPECTED_STATE_ERR, format(
+ new TransactionException(TX_FAILED_READ_WRITE_OPERATION_ERR, format(
Review Comment:
I guess not, because this error is possible on PartitionReplicaListener side (after enlistment) and we dont enlist anything into RO transactions as well
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1475969285
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -1944,7 +1944,7 @@ private <T> CompletableFuture<T> appendTxCommand(UUID txId, RequestType cmdType,
});
if (cleanupReadyFut.isCompletedExceptionally()) {
- return failedFuture(new TransactionException(TX_FAILED_READ_WRITE_OPERATION_ERR, "Transaction is already finished."));
+ return failedFuture(new TransactionException(TX_ALREADY_FINISHED_ERR, "Transaction is already finished."));
Review Comment:
Is that true? Does it really means that if cleanupReadyFut was completed exceptionally then transaction was already finished?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1472779634
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2157,150 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
+
+ finishLatch.countDown();
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ assertThat(futFinishes, willSucceedFast());
+ assertThat(futEnlists, willSucceedFast());
+
+ assertEquals(threadNum, enlistExceptions.size());
+
+ for (var e : enlistExceptions) {
+ assertInstanceOf(TransactionException.class, e);
+ }
+
+ assertTrue(CollectionUtils.nullOrEmpty(txManager(accounts).lockManager().locks(txId)));
+ }
+
+ /**
+ * Test trying to finish a read only tx in multiple threads simultaneously.
+ */
+ @RepeatedTest(10)
+ public void testReadOnlyTransactionMultiThreadedFinish() {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin(new TransactionOptions().readOnly(true));
+
+ rv.get(tx, makeKey(1));
+
+ int threadNum = Runtime.getRuntime().availableProcessors();
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
Review Comment:
removed
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1477971660
##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/IgniteTestUtils.java:
##########
@@ -282,6 +285,39 @@ public static Throwable assertThrows(
return throwable;
}
+ /**
+ * Checks whether runnable throws the correct {@link IgniteException}, which is itself of a specified class.
+ *
+ * @param expectedClass Expected exception class.
+ * @param expectedErrorCode Expected error code of the {@link IgniteException}.
+ * @param run Runnable to check.
+ * @param errorMessageFragment Fragment of the error text in the expected exception, {@code null} if not to be checked.
+ * @return Thrown throwable.
+ */
+ public static Throwable assertThrowsWithCode(
Review Comment:
Why it's not assertThrowsWith**Cause**?
What I mean here is that I'd expect 3 overloaded versions. Where less specific ones will call generalized with null as code or message just like current assertThrowsWithCause do
```
public static Throwable assertThrowsWithCause(
RunnableX run,
Class<? extends Throwable> cls
) {
return assertThrowsWithCause(run, cls, null);
}
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1472765019
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2157,150 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
+
+ finishLatch.countDown();
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ assertThat(futFinishes, willSucceedFast());
+ assertThat(futEnlists, willSucceedFast());
+
+ assertEquals(threadNum, enlistExceptions.size());
+
+ for (var e : enlistExceptions) {
+ assertInstanceOf(TransactionException.class, e);
Review Comment:
done
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1472785180
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2157,150 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
+
+ finishLatch.countDown();
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ assertThat(futFinishes, willSucceedFast());
+ assertThat(futEnlists, willSucceedFast());
+
+ assertEquals(threadNum, enlistExceptions.size());
+
+ for (var e : enlistExceptions) {
+ assertInstanceOf(TransactionException.class, e);
+ }
+
+ assertTrue(CollectionUtils.nullOrEmpty(txManager(accounts).lockManager().locks(txId)));
+ }
+
+ /**
+ * Test trying to finish a read only tx in multiple threads simultaneously.
+ */
+ @RepeatedTest(10)
+ public void testReadOnlyTransactionMultiThreadedFinish() {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin(new TransactionOptions().readOnly(true));
+
+ rv.get(tx, makeKey(1));
+
+ int threadNum = Runtime.getRuntime().availableProcessors();
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, -1);
Review Comment:
They are not forbidden right now so such checks are useless. I can add a TODO here.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1474706785
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2158,178 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1, false);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommitEmpty() {
+ testTransactionMultiThreadedFinish(1, true);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0, false);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollbackEmpty() {
+ testTransactionMultiThreadedFinish(0, true);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1, false);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixedEmpty() {
+ testTransactionMultiThreadedFinish(-1, true);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ * @param checkEmptyTx Whether the tx should be empty on finishing (no enlisted operations).
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode, boolean checkEmptyTx) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ if (!checkEmptyTx) {
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+ }
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
+
+ finishLatch.countDown();
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ assertThat(futFinishes, willSucceedFast());
+ assertThat(futEnlists, willSucceedFast());
+
+ assertEquals(threadNum, enlistExceptions.size());
+
+ for (var e : enlistExceptions) {
+ try {
+ assertInstanceOf(TransactionException.class, e);
Review Comment:
Why it's possible to have two different messages here?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1477971660
##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/IgniteTestUtils.java:
##########
@@ -282,6 +285,39 @@ public static Throwable assertThrows(
return throwable;
}
+ /**
+ * Checks whether runnable throws the correct {@link IgniteException}, which is itself of a specified class.
+ *
+ * @param expectedClass Expected exception class.
+ * @param expectedErrorCode Expected error code of the {@link IgniteException}.
+ * @param run Runnable to check.
+ * @param errorMessageFragment Fragment of the error text in the expected exception, {@code null} if not to be checked.
+ * @return Thrown throwable.
+ */
+ public static Throwable assertThrowsWithCode(
Review Comment:
Why it's not assertThrowsWith**Cause**?
What I mean here is that I'd expect 3 overloaded versions. Where less specific ones will call generalized with null as code or message just like current assertThrowsWithCause do
```
public static Throwable assertThrowsWithCause(
RunnableX run,
Class<? extends Throwable> cls
) {
return assertThrowsWithCause(run, cls, null);
}
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1474662747
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2158,178 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1, false);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommitEmpty() {
+ testTransactionMultiThreadedFinish(1, true);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0, false);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollbackEmpty() {
+ testTransactionMultiThreadedFinish(0, true);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1, false);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixedEmpty() {
+ testTransactionMultiThreadedFinish(-1, true);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ * @param checkEmptyTx Whether the tx should be empty on finishing (no enlisted operations).
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode, boolean checkEmptyTx) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ if (!checkEmptyTx) {
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+ }
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
+
+ finishLatch.countDown();
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ assertThat(futFinishes, willSucceedFast());
+ assertThat(futEnlists, willSucceedFast());
+
+ assertEquals(threadNum, enlistExceptions.size());
+
+ for (var e : enlistExceptions) {
+ try {
+ assertInstanceOf(TransactionException.class, e);
Review Comment:
I believe that we either already have an utility method to fully verify exception with class, code and message or may create it if we don't.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1477885587
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2178,13 +2340,13 @@ protected void testTransactionAlreadyFinished(boolean commit, boolean checkLocks
assertTrue(ex.getMessage().contains("Transaction is already finished."));
ex = assertThrows(TransactionException.class, () -> accountsRv.delete(tx, makeKey(1)));
- assertTrue(ex.getMessage().contains("Failed to enlist"));
+ assertTrue(ex.getMessage().contains("Transaction is already finished"));
Review Comment:
yes, replaced this
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1470662339
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2157,150 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
Review Comment:
So, seems that you only check empty transactions finish, is that correct? I'd rather make it variable, meaning that we may add one more boolean parameter emptyTransaction to testTransactionMultiThreadedFinish() and if it's false perform some inserts within tx scope before initial finish.
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2157,150 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
+
+ finishLatch.countDown();
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ assertThat(futFinishes, willSucceedFast());
+ assertThat(futEnlists, willSucceedFast());
+
+ assertEquals(threadNum, enlistExceptions.size());
+
+ for (var e : enlistExceptions) {
+ assertInstanceOf(TransactionException.class, e);
+ }
+
+ assertTrue(CollectionUtils.nullOrEmpty(txManager(accounts).lockManager().locks(txId)));
+ }
+
+ /**
+ * Test trying to finish a read only tx in multiple threads simultaneously.
+ */
+ @RepeatedTest(10)
+ public void testReadOnlyTransactionMultiThreadedFinish() {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin(new TransactionOptions().readOnly(true));
+
+ rv.get(tx, makeKey(1));
+
+ int threadNum = Runtime.getRuntime().availableProcessors();
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, -1);
Review Comment:
I believe that we should also check get/scan "enlist" attempts into finished RO transaction.
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2157,150 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
+
+ finishLatch.countDown();
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ assertThat(futFinishes, willSucceedFast());
+ assertThat(futEnlists, willSucceedFast());
+
+ assertEquals(threadNum, enlistExceptions.size());
+
+ for (var e : enlistExceptions) {
+ assertInstanceOf(TransactionException.class, e);
Review Comment:
Let's also assert the the code and message, at least stable part of a message.
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2157,150 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
+
+ finishLatch.countDown();
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ assertThat(futFinishes, willSucceedFast());
+ assertThat(futEnlists, willSucceedFast());
+
+ assertEquals(threadNum, enlistExceptions.size());
+
+ for (var e : enlistExceptions) {
+ assertInstanceOf(TransactionException.class, e);
+ }
+
+ assertTrue(CollectionUtils.nullOrEmpty(txManager(accounts).lockManager().locks(txId)));
+ }
+
+ /**
+ * Test trying to finish a read only tx in multiple threads simultaneously.
+ */
+ @RepeatedTest(10)
+ public void testReadOnlyTransactionMultiThreadedFinish() {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin(new TransactionOptions().readOnly(true));
+
+ rv.get(tx, makeKey(1));
+
+ int threadNum = Runtime.getRuntime().availableProcessors();
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
Review Comment:
There's no finishLatch.await() in this test.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -132,10 +132,7 @@ private boolean hasTxFinalizationBegun() {
/** {@inheritDoc} */
@Override
protected CompletableFuture<Void> finish(boolean commit) {
- if (hasTxFinalizationBegun()) {
Review Comment:
Could you please explain why it was removed?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1470735337
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -132,10 +132,7 @@ private boolean hasTxFinalizationBegun() {
/** {@inheritDoc} */
@Override
protected CompletableFuture<Void> finish(boolean commit) {
- if (hasTxFinalizationBegun()) {
Review Comment:
In the finishing flow, we set a transaction state and then assign the future. Hence, we may check the state, but the future is still null.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1478032420
##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/IgniteTestUtils.java:
##########
@@ -322,6 +326,35 @@ public static Throwable assertThrowsWithCause(
throw new AssertionError("Exception has not been thrown.");
}
+ /**
+ * Checks the given {@link IgniteException} that it is the instance of the correct class, has the correct code and message.
+ * If the exception is incorrect, {@link AssertionError} is thrown and the incorrect exception is added to suppressed exceptions.
+ *
+ * @param expectedClass Expected class of the exception.
+ * @param expectedErrorCode Expected error code of the {@link IgniteException}.
+ * @param expectedMessageSubstring A substring that must be contained in the error message of the exception.
+ * @param exception Exception to check.
+ */
+ public static void assertExceptionIsExpected(
Review Comment:
Why it's not assertThrowsWith**Cause**?
What I mean here is that I'd expect 3 overloaded versions. Where less specific ones will call generalized with null as code or message just like current assertThrowsWithCause do
```
public static Throwable assertThrowsWithCause(
RunnableX run,
Class<? extends Throwable> cls
) {
return assertThrowsWithCause(run, cls, null);
}
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc merged PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1474685812
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -2151,12 +2158,178 @@ public void testYoungerTransactionThrowsExceptionIfKeyLockedByOlderTransactionWi
assertThrows(TransactionException.class, () -> keyValueView.put(youngNormalTx, 1L, "normal"));
}
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommit() {
+ testTransactionMultiThreadedFinish(1, false);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedCommitEmpty() {
+ testTransactionMultiThreadedFinish(1, true);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollback() {
+ testTransactionMultiThreadedFinish(0, false);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedRollbackEmpty() {
+ testTransactionMultiThreadedFinish(0, true);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixed() {
+ testTransactionMultiThreadedFinish(-1, false);
+ }
+
+ @RepeatedTest(10)
+ public void testTransactionMultiThreadedMixedEmpty() {
+ testTransactionMultiThreadedFinish(-1, true);
+ }
+
+ /**
+ * Test trying to finish a tx in multiple threads simultaneously, and enlist new operations right after the first finish.
+ *
+ * @param finishMode 1 is commit, 0 is rollback, otherwise random outcome.
+ * @param checkEmptyTx Whether the tx should be empty on finishing (no enlisted operations).
+ */
+ private void testTransactionMultiThreadedFinish(int finishMode, boolean checkEmptyTx) {
+ var rv = accounts.recordView();
+
+ rv.upsert(null, makeValue(1, 1.));
+
+ Transaction tx = igniteTransactions.begin();
+
+ var txId = ((ReadWriteTransactionImpl) tx).id();
+
+ log.info("Started transaction {}", txId);
+
+ if (!checkEmptyTx) {
+ rv.upsert(tx, makeValue(1, 100.));
+ rv.upsert(tx, makeValue(2, 200.));
+ }
+
+ int threadNum = Runtime.getRuntime().availableProcessors() * 5;
+
+ CyclicBarrier b = new CyclicBarrier(threadNum);
+ CountDownLatch finishLatch = new CountDownLatch(1);
+
+ List<Exception> enlistExceptions = synchronizedList(new ArrayList<>());
+
+ var futEnlists = runMultiThreadedAsync(() -> {
+ finishLatch.await();
+ var rnd = ThreadLocalRandom.current();
+
+ try {
+ if (rnd.nextBoolean()) {
+ rv.upsert(tx, makeValue(2, 200.));
+ } else {
+ rv.get(tx, makeKey(1));
+ }
+ } catch (Exception e) {
+ enlistExceptions.add(e);
+ }
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ var futFinishes = runMultiThreadedAsync(() -> {
+ b.await();
+
+ finishTx(tx, finishMode);
+
+ finishLatch.countDown();
+
+ return null;
+ }, threadNum, "txCommitTestThread");
+
+ assertThat(futFinishes, willSucceedFast());
+ assertThat(futEnlists, willSucceedFast());
+
+ assertEquals(threadNum, enlistExceptions.size());
+
+ for (var e : enlistExceptions) {
+ try {
+ assertInstanceOf(TransactionException.class, e);
Review Comment:
assertThrowsWithCause? I think it's not what is needed, it checks a Runnable and doesnt have multiple acceptable messages
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1472780722
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -132,10 +132,7 @@ private boolean hasTxFinalizationBegun() {
/** {@inheritDoc} */
@Override
protected CompletableFuture<Void> finish(boolean commit) {
- if (hasTxFinalizationBegun()) {
Review Comment:
It fixes a race between new enlist, tx finish and the change of `finishFuture` field
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20894 Add tests for ensuring transactional guarantees [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3092:
URL: https://github.com/apache/ignite-3/pull/3092#discussion_r1474659869
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -520,7 +520,7 @@ private <R> CompletableFuture<R> trackingInvoke(
// Track only write requests from explicit transactions.
if (!txManager.addInflight(tx.id())) {
return failedFuture(
- new TransactionException(TX_UNEXPECTED_STATE_ERR, format(
+ new TransactionException(TX_FAILED_READ_WRITE_OPERATION_ERR, format(
Review Comment:
Should we name it TX_ENLISTMENT_ERR or similar in order not to update it when we will expand the logic over RO transactions?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org