rpuch commented on code in PR #5209:
URL: https://github.com/apache/ignite-3/pull/5209#discussion_r1987399432


##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxManagerImpl.java:
##########
@@ -416,34 +416,49 @@ private InternalTransaction beginBusy(
 
         if (readOnly) {
             HybridTimestamp beginTimestamp = clockService.now();
-
-            UUID txId = 
transactionIdGenerator.transactionIdFor(beginTimestamp, options.priority());
-
-            tx = beginReadOnlyTransaction(timestampTracker, beginTimestamp, 
txId, implicit, options);
+            tx = beginReadOnlyTransaction(timestampTracker, beginTimestamp, 
implicit, options);
         } else {
             HybridTimestamp beginTimestamp = 
createBeginTimestampWithIncrementRwTxCounter();
-
-            UUID txId = 
transactionIdGenerator.transactionIdFor(beginTimestamp, options.priority());
-
-            // TODO: RW timeouts will be supported in 
https://issues.apache.org/jira/browse/IGNITE-24244
-            //  long timeout = options.timeoutMillis() == 0 ? 
txConfig.readWriteTimeout().value() : options.timeoutMillis();
-            long timeout = 3_000;
-
-            tx = new ReadWriteTransactionImpl(this, timestampTracker, txId, 
localNodeId, implicit, timeout);
+            tx = beginReadWriteTransaction(timestampTracker, beginTimestamp, 
implicit, options);
         }
 
         txStateVolatileStorage.initialize(tx);
 
         return tx;
     }
 
+    private ReadWriteTransactionImpl beginReadWriteTransaction(
+            HybridTimestampTracker timestampTracker,
+            HybridTimestamp beginTimestamp,
+            boolean implicit,
+            InternalTxOptions options) {
+
+        UUID txId = transactionIdGenerator.transactionIdFor(beginTimestamp, 
options.priority());

Review Comment:
   This one is a pretty important zero. :) I mean, it is really easy to forget 
that it means 'take the default from server config' and not actual zero and not 
'no timeout at all'. As for where to put it, the `core` module is accessible 
both by server and client. I don't think it's the *best* place, but it will 
work for now.



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

Reply via email to