denis-chudov commented on code in PR #2757:
URL: https://github.com/apache/ignite-3/pull/2757#discussion_r1374212479
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -56,6 +62,12 @@ public class ReadWriteTransactionImpl extends
IgniteAbstractTransactionImpl {
/** A partition which stores the transaction state. */
private volatile TablePartitionId commitPart;
+ /** The lock protects the transaction topology from concurrent
modification during finishing. */
+ private final ReentrantReadWriteLock enlistPartLock = new
ReentrantReadWriteLock();
Review Comment:
Lets choose human-understandable names :) What about "enlistPartitionLock"?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -90,12 +102,57 @@ public IgniteBiTuple<ClusterNode, Long>
enlistedNodeAndTerm(TablePartitionId par
/** {@inheritDoc} */
@Override
public IgniteBiTuple<ClusterNode, Long> enlist(TablePartitionId
tablePartitionId, IgniteBiTuple<ClusterNode, Long> nodeAndTerm) {
- return enlisted.computeIfAbsent(tablePartitionId, k -> nodeAndTerm);
+ checkEnlistReady();
+
+ try {
+ enlistPartLock.readLock().lock();
+
+ checkEnlistReady();
+
+ return enlisted.computeIfAbsent(tablePartitionId, k ->
nodeAndTerm);
+ } finally {
+ enlistPartLock.readLock().unlock();
+ }
+ }
+
+ /**
+ * Checks that this transaction was not finished and will be able to
enlist another partition.
+ */
+ private void checkEnlistReady() {
+ if (isFinalState(state())) {
+ throw new TransactionException(
+ TX_FAILED_READ_WRITE_OPERATION_ERR,
+ format("Transaction is already finished [id={},
state={}].", id(), state()));
+ }
}
/** {@inheritDoc} */
@Override
protected CompletableFuture<Void> finish(boolean commit) {
+ if (isFinalState(state())) {
+ return finishFuture;
+ }
+
+ try {
+ enlistPartLock.writeLock().lock();
+
+ if (!isFinalState(state())) {
+ finishFuture = finishInternal(commit);
Review Comment:
2 spaces instead of one
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -90,12 +102,57 @@ public IgniteBiTuple<ClusterNode, Long>
enlistedNodeAndTerm(TablePartitionId par
/** {@inheritDoc} */
@Override
public IgniteBiTuple<ClusterNode, Long> enlist(TablePartitionId
tablePartitionId, IgniteBiTuple<ClusterNode, Long> nodeAndTerm) {
- return enlisted.computeIfAbsent(tablePartitionId, k -> nodeAndTerm);
+ checkEnlistReady();
+
+ try {
+ enlistPartLock.readLock().lock();
+
+ checkEnlistReady();
+
+ return enlisted.computeIfAbsent(tablePartitionId, k ->
nodeAndTerm);
+ } finally {
+ enlistPartLock.readLock().unlock();
+ }
+ }
+
+ /**
+ * Checks that this transaction was not finished and will be able to
enlist another partition.
+ */
+ private void checkEnlistReady() {
+ if (isFinalState(state())) {
+ throw new TransactionException(
+ TX_FAILED_READ_WRITE_OPERATION_ERR,
+ format("Transaction is already finished [id={},
state={}].", id(), state()));
+ }
}
/** {@inheritDoc} */
@Override
protected CompletableFuture<Void> finish(boolean commit) {
+ if (isFinalState(state())) {
+ return finishFuture;
+ }
+
+ try {
+ enlistPartLock.writeLock().lock();
+
+ if (!isFinalState(state())) {
+ finishFuture = finishInternal(commit);
+ }
+
+ return finishFuture;
+ } finally {
+ enlistPartLock.writeLock().unlock();
+ }
+ }
+
+ /**
+ * Internal method to finishing this transaction.
Review Comment:
```suggestion
* Internal method for finishing this transaction.
```
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########
@@ -152,7 +152,6 @@ public void checkTimestampOperations() {
/** Check transaction change status with erroneous statements. */
@Test
- // TODO should be removed after
https://issues.apache.org/jira/browse/IGNITE-20534 is fixed.
public void testTxStateChangedOnErroneousOp() {
Review Comment:
seems there was an intention to remove the test after the fix (but I still
cant get why), and you just removed todo - what is correct 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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
