rpuch commented on code in PR #5093:
URL: https://github.com/apache/ignite-3/pull/5093#discussion_r1935333695
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -368,9 +362,12 @@ private <R> CompletableFuture<R> enlistInTx(
return postEnlist(fut, false, actualTx,
actualTx.implicit()).handle((r, e) -> {
if (e != null) {
if (actualTx.implicit()) {
+ // TODO: IGNITE-24244
+ long timeout = actualTx.isReadOnly() ? actualTx.timeout()
: 10_000;
+
long ts = (txStartTs == null) ?
actualTx.startTimestamp().getPhysical() : txStartTs;
- if (exceptionAllowsImplicitTxRetry(e) &&
coarseCurrentTimeMillis() - ts < implicitTransactionTimeout) {
+ if (exceptionAllowsImplicitTxRetry(e) &&
coarseCurrentTimeMillis() - ts < timeout) {
Review Comment:
How about extracting the `if` condition to a method? It's used a few times
in this same form
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -486,9 +483,12 @@ private <T> CompletableFuture<T> enlistInTx(
return postEnlist(fut, actualTx.implicit() && !singlePart, actualTx,
full).handle((r, e) -> {
if (e != null) {
if (actualTx.implicit()) {
+ // TODO: IGNITE-24244
+ long timeout = actualTx.isReadOnly() ? actualTx.timeout()
: 3_000;
Review Comment:
Another constant, please
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -368,9 +362,12 @@ private <R> CompletableFuture<R> enlistInTx(
return postEnlist(fut, false, actualTx,
actualTx.implicit()).handle((r, e) -> {
if (e != null) {
if (actualTx.implicit()) {
+ // TODO: IGNITE-24244
+ long timeout = actualTx.isReadOnly() ? actualTx.timeout()
: 10_000;
Review Comment:
Let's introduce a temporary constant for 10000
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/IgniteAbstractTransactionImpl.java:
##########
@@ -55,6 +55,9 @@ public abstract class IgniteAbstractTransactionImpl
implements InternalTransacti
/** Implicit transaction flag. */
private final boolean implicit;
+ /** Transaction timeout. */
+ protected long timeout;
Review Comment:
Should it be final?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/IgniteAbstractTransactionImpl.java:
##########
@@ -63,19 +66,22 @@ public abstract class IgniteAbstractTransactionImpl
implements InternalTransacti
* @param observableTsTracker Observation timestamp tracker.
* @param coordinatorId Transaction coordinator inconsistent ID.
* @param implicit True for an implicit transaction, false for an ordinary
one.
+ * @param timeout Transaction timeout.
Review Comment:
```suggestion
* @param timeout Transaction timeout in milliseconds.
```
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java:
##########
@@ -28,23 +28,20 @@
*/
@Config
public class TransactionConfigurationSchema {
- /** Default checking transaction interval. */
- public static final long DEFAULT_ABANDONED_CHECK_TS = 5_000;
-
/** How often abandoned transactions are searched for (milliseconds). */
@Range(min = 0)
@Value(hasDefault = true)
- public final long abandonedCheckTs = DEFAULT_ABANDONED_CHECK_TS;
+ public final long abandonedCheckTs = 5_000;
- /** Default transaction timeout (milliseconds). */
+ /** Default timeout for read-only transactions. */
@Range(min = 1)
@Value(hasDefault = true)
- public final long timeout = 10_000;
Review Comment:
This still looks like a breaking change. Let's consult @ptupitsyn whether we
are allowed to directly remove a configuration property
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxManagerImpl.java:
##########
@@ -475,7 +483,7 @@ private ReadOnlyTransactionImpl beginReadOnlyTransaction(
}
private long roExpirationPhysicalTimeFor(HybridTimestamp beginTimestamp,
InternalTxOptions options) {
- long effectiveTimeoutMillis = options.timeoutMillis() == 0 ?
defaultTransactionTimeoutMillis() : options.timeoutMillis();
+ long effectiveTimeoutMillis = options.timeoutMillis() == 0 ?
defaultReadOnlyTransactionTimeoutMillis() : options.timeoutMillis();
Review Comment:
This calculation is repeated at least twice. Let's extract it to a method
--
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]
