IGNITE-24242 Add RO/RW Transaction Timeout configuration schema by PakhomovAlexander · Pull Request #5093 · apache/ignite-3

PakhomovAlexander · 2025-01-21T20:37:25Z

https://issues.apache.org/jira/browse/IGNITE-24242

...le/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

rpuch · 2025-01-23T07:08:36Z

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

+    public final long roTimeout = 10_000;
+
+    @Range(min = 1)
+    @Value(hasDefault = true)
+    public final long rwTimeout = 3_000;


There is already timeout which defines default timeouts. Now, rwTimeout and roTimeout are added. Do they serve the same purpose? If yes, timeout should probably be removed.

But if it's removed, it's a breaking change (as 3.0 code freeze has already happened) and it should be handled in a different way, probably by declaring it as deprecated and, maybe, using the existing value for some time?

I believe we still can do breaking changes in configuration APIs.

timeout removed

sanpwc · 2025-01-23T07:31:55Z

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

    private final PartitionReplicaLifecycleManager partitionReplicaLifecycleManager;

-    private long implicitTransactionTimeout;
+    private long roTransactionTimeout;


Tx timeout is tx specific attribute, meaning that tx1 will have 100ms as timeout and tx2 that is running at the same time 2000ms. In that case why roTransactionTimeout and rwTransactionTimeout are here?
Besides that, it's just an encapsulation leak - simply TableManager should not know about such things.
I'd rather say that on tx creation we should populate TransacionOptions with default timeout from cfg if it's not specified by the user and within tx flow use timeout from the tx context - no need to propagate txTimeout to InternalTableImpl etc.

Do you agree that implicitTransactionTimeout should be removed with this PR? If yes, then it is clear why these options are there. If we should keep implicitTransactionTimeout then, probably, I should rollback the change.

I suppose we should remove implicitTransactionTimeout and refactor TableManager as you suggested. Am I right?

+1 to remove implicitTransactionTimeout

In that case why roTransactionTimeout and rwTransactionTimeout are here?

I thought these properties define default TX timeouts. A default is global and not specific to a transaction.

I suppose we should remove implicitTransactionTimeout and refactor TableManager as you suggested. Am I right?

Yes.

...le/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java

sanpwc · 2025-01-23T07:45:07Z

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

+
+    @Range(min = 1)
+    @Value(hasDefault = true)
+    public final long maxRoTimeout = Long.MAX_VALUE;


Why it's not equal to dataAvailabilityTime?

(deleted as irrelevant)

property deleted

sanpwc · 2025-01-23T07:47:25Z

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

+
+    @Range(min = 1)
+    @Value(hasDefault = true)
+    public final long maxRwTimeout = Long.MAX_VALUE;


Is it too much for default max value?

property deleted

sanpwc · 2025-01-23T07:48:15Z

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

+
+    @Range(min = 1)
+    @Value(hasDefault = true)
+    public final long maxRoTimeout = Long.MAX_VALUE;


Here and there: for such properties it's reasonable to have milis(or any other) suffix.

There was a proposal to switch duration-related properties type to Duration and allow the user specify units in the config. Having this in mind, it seems that we should not include units in property names.

My comment only makes sense if we still want to have that feature in the configuration.

property deleted

...le/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java

ascherbakoff · 2025-01-23T07:47:40Z

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

+
+    @Range(min = 1)
+    @Value(hasDefault = true)
+    public final long maxRoTimeout = Long.MAX_VALUE;


What is the purpose of
minRoTimeout maxRoTimeout minRwTimeout maxRwTimeout ?
It doesn't make sense to me.

I agree. Will drop these properties.

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

ascherbakoff · 2025-01-23T07:53:45Z

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

+    public final long roTimeout = 10_000;
+
+    @Range(min = 1)
+    @Value(hasDefault = true)
+    public final long rwTimeout = 3_000;


I believe we still can do breaking changes in configuration APIs.

ascherbakoff · 2025-01-23T08:02:27Z

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

+
+    @Range(min = 1)
+    @Value(hasDefault = true)
+    public final long roTimeout = 10_000;


How these defaults are calculated?
They seem too low.
I suggest 10 minutes for RO txns, and 30 seconds for RW txns

Good suggestion.

…e updated, we will use up-to-dated values.

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

…tx/configuration/TransactionConfigurationSchema.java Co-authored-by: Roman Puchkovskiy <roman.puchkovskiy@gmail.com>

…NITE-24242

(will be changed in the next ticket)

...le/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java

rpuch · 2025-01-30T10:03:25Z

...le/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java

                    long ts = (txStartTs == null) ? actualTx.startTimestamp().getPhysical() : txStartTs;

-                    if (exceptionAllowsImplicitTxRetry(e) && coarseCurrentTimeMillis() - ts < implicitTransactionTimeout) {
+                    if (exceptionAllowsImplicitTxRetry(e) && coarseCurrentTimeMillis() - ts < timeout) {


How about extracting the if condition to a method? It's used a few times in this same form

...le/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java

rpuch · 2025-01-30T10:06:32Z

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java

+    /** Default timeout for read-only transactions. */
    @Range(min = 1)
    @Value(hasDefault = true)
-    public final long timeout = 10_000;


This still looks like a breaking change. Let's consult @ptupitsyn whether we are allowed to directly remove a configuration property

Good catch. But why should we consult Pavel about changing the property we've added a couple of weeks ago?

Because 3.0.0 already contains transaction.timeout config property. If we just remove it, then our next release will potentially break them.

...sactions/src/main/java/org/apache/ignite/internal/tx/impl/IgniteAbstractTransactionImpl.java

rpuch · 2025-01-30T10:11:26Z

modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxManagerImpl.java


    private long roExpirationPhysicalTimeFor(HybridTimestamp beginTimestamp, InternalTxOptions options) {
-        long effectiveTimeoutMillis = options.timeoutMillis() == 0 ? defaultTransactionTimeoutMillis() : options.timeoutMillis();
+        long effectiveTimeoutMillis = options.timeoutMillis() == 0 ? defaultReadOnlyTransactionTimeoutMillis() : options.timeoutMillis();


This calculation is repeated at least twice. Let's extract it to a method

The next ticket will change this, don't you mind to leave this as is?

I'm ok with this if this will not be forgotten :)

sanpwc · 2025-01-30T10:54:13Z

modules/client/src/test/java/org/apache/ignite/client/fakes/FakeTxManager.java

+
+            @Override
+            public long timeout() {
+                return 10_000;


10_000 is used for the same purpose in multiple places. Should we extract it somewhere?

The next ticket will change this, don't you mind to leave this as is?

…tx/impl/IgniteAbstractTransactionImpl.java Co-authored-by: Roman Puchkovskiy <roman.puchkovskiy@gmail.com>

rpuch

The only concern that still remains from my side is the compatibility issue, let's sort it out

IGNITE-24242 Add RO/RW Transaction Timeout configuration schema

68f1fa8

PakhomovAlexander force-pushed the IGNITE-24242 branch from 2b826b9 to 68f1fa8 Compare January 22, 2025 14:20

rpuch reviewed Jan 23, 2025

View reviewed changes

sanpwc reviewed Jan 23, 2025

View reviewed changes

ascherbakoff requested changes Jan 23, 2025

View reviewed changes

PakhomovAlexander added 2 commits January 23, 2025 14:48

Review fixes: javadoc and configurations naming

2e7c24c

Use suppliers for ro/rw timeouts in TableManager, so if the config ar…

c4fcc59

…e updated, we will use up-to-dated values.

ascherbakoff approved these changes Jan 24, 2025

View reviewed changes

PakhomovAlexander added 2 commits January 24, 2025 18:14

Use timeout from TransationOptions rather than from configuration.

03a0274

Fix config in test

7dcb643

rpuch reviewed Jan 27, 2025

View reviewed changes

...rc/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java Outdated Show resolved Hide resolved

PakhomovAlexander and others added 13 commits January 27, 2025 16:52

Fix test

1e21f92

Update modules/transactions/src/main/java/org/apache/ignite/internal/…

a34ded1

…tx/configuration/TransactionConfigurationSchema.java Co-authored-by: Roman Puchkovskiy <roman.puchkovskiy@gmail.com>

Merge branch 'IGNITE-24242' of github.com:unisonteam/ignite-3 into IG…

9b6203a

…NITE-24242

Use TimeUnit for defaults in configuration

17f37e4

Fix comment in constructor

4340ec3

Fix test, add todo

db2fcef

Fix checkstyle

71ac52d

Add todos for the next ticket

ba756bc

Change default timeout for rw transactions to make tests pass

c05476d

fix

fa7f5df

Fix

cc9cb48

Change default RW timeout to make test pass

ae00d30

(will be changed in the next ticket)

Fix test

e1ee039

rpuch reviewed Jan 30, 2025

View reviewed changes

sanpwc approved these changes Jan 30, 2025

View reviewed changes

PakhomovAlexander and others added 2 commits January 30, 2025 14:15

Update modules/transactions/src/main/java/org/apache/ignite/internal/…

2f1e409

…tx/impl/IgniteAbstractTransactionImpl.java Co-authored-by: Roman Puchkovskiy <roman.puchkovskiy@gmail.com>

IGNITE-24242 Add RO/RW Transaction Timeout configuration schema

c0b0e40

rpuch reviewed Jan 30, 2025

View reviewed changes

Fix checkstyle

2f13beb

PakhomovAlexander merged commit 25f5e78 into apache:main Jan 30, 2025
1 check passed

Conversation

PakhomovAlexander commented Jan 21, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpuch Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpuch Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpuch left a comment

Choose a reason for hiding this comment

Uh oh!

rpuch Jan 27, 2025 •

edited

Loading

rpuch Jan 23, 2025 •

edited

Loading