[GitHub] [ignite-3] ptupitsyn commented on a diff in pull request #1503: IGNITE-18324 Calling tx.rollback() or tx.commit() is not failed for committed transaction.

[GitBox]

ptupitsyn commented on code in PR #1503:
URL: https://github.com/apache/ignite-3/pull/1503#discussion_r1066699108


##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -87,7 +84,9 @@ public void commit() throws TransactionException {
     /** {@inheritDoc} */
     @Override
     public CompletableFuture<Void> commitAsync() {
-        setState(STATE_COMMITTED);
+        if (!state.compareAndSet(STATE_OPEN, STATE_CLOSE)) {

Review Comment:
   We can probably replace `AtomicInteger` with `AtomicBoolean` now since there 
are only 2 states.



##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientTransactionsTest.java:
##########
@@ -199,21 +200,23 @@ void testAccessLockedKeyTimesOut() {
     }
 
     @Test
-    void testCommitRollbackSameTxThrows() {
+    void testCommitRollbackSameTxNotThrows() {

Review Comment:
   ```suggestion
       void testCommitRollbackSameTxDoesNotThrow() {
   ```



##########
modules/table/src/test/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -121,10 +122,29 @@ public abstract class TxAbstractTest extends 
IgniteAbstractTest {
     public abstract void before() throws Exception;
 
     @Test
-    public void testDeleteUpsertCommit() throws TransactionException {
-        deleteUpsert().commit();
+    public void testCommitRollbackSameTxNotThrows() throws 
TransactionException {

Review Comment:
   ```suggestion
       public void testCommitRollbackSameTxDoesNotThrow() throws 
TransactionException {
   ```



##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientTransactionsTest.java:
##########
@@ -199,21 +200,23 @@ void testAccessLockedKeyTimesOut() {
     }
 
     @Test
-    void testCommitRollbackSameTxThrows() {
+    void testCommitRollbackSameTxNotThrows() {
         Transaction tx = client().transactions().begin();
         tx.commit();
 
-        TransactionException ex = assertThrows(TransactionException.class, 
tx::rollback);
-        assertThat(ex.getMessage(), containsString("Transaction is already 
committed"));
+        assertDoesNotThrow(tx::rollback, "Unexpected exception was thrown.");
+        assertDoesNotThrow(tx::commit, "Unexpected exception was thrown.");
+        assertDoesNotThrow(tx::rollback, "Unexpected exception was thrown.");
     }
 
     @Test
-    void testRollbackCommitSameTxThrows() {
+    void testRollbackCommitSameTxNotThrows() {

Review Comment:
   ```suggestion
       void testRollbackCommitSameTxDoesNotThrow() {
   ```



##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -34,11 +34,8 @@ public class ClientTransaction implements Transaction {
     /** Open state. */
     private static final int STATE_OPEN = 0;
 
-    /** Committed state. */
-    private static final int STATE_COMMITTED = 1;
-
-    /** Rolled back state. */
-    private static final int STATE_ROLLED_BACK = 2;
+    /** Close state. */
+    private static final int STATE_CLOSE = 1;

Review Comment:
   ```suggestion
       private static final int STATE_CLOSED = 1;
   ```



##########
modules/table/src/test/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -121,10 +122,29 @@ public abstract class TxAbstractTest extends 
IgniteAbstractTest {
     public abstract void before() throws Exception;
 
     @Test
-    public void testDeleteUpsertCommit() throws TransactionException {
-        deleteUpsert().commit();
+    public void testCommitRollbackSameTxNotThrows() throws 
TransactionException {
+        InternalTransaction tx = (InternalTransaction) 
igniteTransactions.begin();
 
-        assertEquals(200., accounts.recordView().get(null, 
makeKey(1)).doubleValue("balance"));
+        accounts.recordView().upsert(tx, makeValue(1, 100.));
+
+        tx.commit();
+
+        assertDoesNotThrow(tx::rollback, "Unexpected exception was thrown.");
+        assertDoesNotThrow(tx::commit, "Unexpected exception was thrown.");
+        assertDoesNotThrow(tx::rollback, "Unexpected exception was thrown.");
+    }
+
+    @Test
+    public void testRollbackCommitSameTxNotThrows() throws 
TransactionException {

Review Comment:
   ```suggestion
       public void testRollbackCommitSameTxDoesNotThrow() throws 
TransactionException {
   ```



##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -981,7 +989,7 @@ private CompletableFuture<Void> 
processTxFinishAction(TxFinishReplicaRequest req
      * @param commit True is the transaction is committed, false otherwise.
      * @return Future to wait of the finish.
      */
-    private CompletableFuture<Object> 
finishTransaction(List<ReplicationGroupId> aggregatedGroupIds, UUID txId, 
boolean commit) {
+    private CompletableFuture<Object> 
finishTransaction(List<ReplicationGroupId> aggregatedGroupIds, UUID txId, 
boolean commit) {//

Review Comment:
   Temp change? Please revert.



-- 
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