IGNITE-24242 Add RO/RW Transaction Timeout configuration schema #5093

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

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

rpuch · 2025-01-23T07:03:56Z

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

+    private final long roTransactionTimeout;
+
+    private final long rwTransactionTimeout;


Should we allow these timeouts to be changed at runtime?

I assume that txTimeout is tx attribute and not tableManager/InternalTableImpl, meaning that different transactions may have different timeouts. Adjustable defaults should populate tx on start. More details in the comment above.

+1. we should

rpuch · 2025-01-23T07:06:37Z

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

    @Range(min = 1)
    @Value(hasDefault = true)
-    public final long implicitTransactionTimeout = 3_000;
+    public final long minRoTimeout = 1;


Javadocs are missing

Abbreviations like RO and RW look ugly in user-facing API. Should we name them readOnlyXXX and readWriteXXX?

+1. we should

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

@@ -418,7 +418,9 @@ public class TableManager implements IgniteTablesInternal, IgniteComponent {

    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

sanpwc · 2025-01-23T07:37:17Z

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

+    private final long roTransactionTimeout;
+
+    private final long rwTransactionTimeout;


I assume that txTimeout is tx attribute and not tableManager/InternalTableImpl, meaning that different transactions may have different timeouts. Adjustable defaults should populate tx on start. More details in the comment above.

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

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

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

+    private final long roTransactionTimeout;
+
+    private final long rwTransactionTimeout;


+1. we should

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.

ascherbakoff · 2025-01-23T07:49:56Z

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

    @Range(min = 1)
    @Value(hasDefault = true)
-    public final long implicitTransactionTimeout = 3_000;
+    public final long minRoTimeout = 1;


+1. we should

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-24242 Add RO/RW Transaction Timeout configuration schema #5093

IGNITE-24242 Add RO/RW Transaction Timeout configuration schema #5093

PakhomovAlexander commented Jan 21, 2025

rpuch Jan 23, 2025

sanpwc Jan 23, 2025

ascherbakoff Jan 23, 2025

rpuch Jan 23, 2025

ascherbakoff Jan 23, 2025

PakhomovAlexander Jan 23, 2025

rpuch Jan 23, 2025

ascherbakoff Jan 23, 2025

PakhomovAlexander Jan 23, 2025

sanpwc Jan 23, 2025

PakhomovAlexander Jan 23, 2025

ascherbakoff Jan 23, 2025

sanpwc Jan 23, 2025

sanpwc Jan 23, 2025

rpuch Jan 23, 2025 •

edited

Loading

PakhomovAlexander Jan 23, 2025

sanpwc Jan 23, 2025

PakhomovAlexander Jan 23, 2025

sanpwc Jan 23, 2025

rpuch Jan 23, 2025

PakhomovAlexander Jan 23, 2025

ascherbakoff Jan 23, 2025

ascherbakoff Jan 23, 2025

PakhomovAlexander Jan 23, 2025

ascherbakoff Jan 23, 2025

ascherbakoff Jan 23, 2025

ascherbakoff Jan 23, 2025

PakhomovAlexander Jan 23, 2025

		private final long roTransactionTimeout;

		private final long rwTransactionTimeout;

IGNITE-24242 Add RO/RW Transaction Timeout configuration schema #5093

Are you sure you want to change the base?

IGNITE-24242 Add RO/RW Transaction Timeout configuration schema #5093

Conversation

PakhomovAlexander commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpuch Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpuch Jan 23, 2025 •

edited

Loading