sanpwc commented on code in PR #5093:
URL: https://github.com/apache/ignite-3/pull/5093#discussion_r1926507416
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java:
##########
@@ -41,10 +41,29 @@ public class TransactionConfigurationSchema {
@Value(hasDefault = true)
public final long timeout = 10_000;
- /** Timeout for implicit transactions (milliseconds). */
@Range(min = 1)
@Value(hasDefault = true)
- public final long implicitTransactionTimeout = 3_000;
+ public final long minRoTimeout = 1;
+
+ @Range(min = 1)
+ @Value(hasDefault = true)
+ public final long maxRoTimeout = Long.MAX_VALUE;
Review Comment:
Why it's not equal to dataAvailabilityTime?
##########
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;
Review Comment:
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.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -198,8 +198,9 @@ public class InternalTableImpl implements InternalTable {
/** Map update guarded by {@link #updatePartitionMapsMux}. */
private volatile Int2ObjectMap<PendingComparableValuesTracker<Long, Void>>
storageIndexTrackerByPartitionId = emptyMap();
- /** Implicit transaction timeout. */
- private final long implicitTransactionTimeout;
+ private final long roTransactionTimeout;
+
+ private final long rwTransactionTimeout;
Review Comment:
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.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java:
##########
@@ -41,10 +41,29 @@ public class TransactionConfigurationSchema {
@Value(hasDefault = true)
public final long timeout = 10_000;
- /** Timeout for implicit transactions (milliseconds). */
@Range(min = 1)
@Value(hasDefault = true)
- public final long implicitTransactionTimeout = 3_000;
+ public final long minRoTimeout = 1;
+
+ @Range(min = 1)
+ @Value(hasDefault = true)
+ public final long maxRoTimeout = Long.MAX_VALUE;
+
+ @Range(min = 1)
+ @Value(hasDefault = true)
+ public final long minRwTimeout = 1;
+
+ @Range(min = 1)
+ @Value(hasDefault = true)
+ public final long maxRwTimeout = Long.MAX_VALUE;
Review Comment:
Is it too much for default max value?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java:
##########
@@ -41,10 +41,29 @@ public class TransactionConfigurationSchema {
@Value(hasDefault = true)
public final long timeout = 10_000;
- /** Timeout for implicit transactions (milliseconds). */
@Range(min = 1)
@Value(hasDefault = true)
- public final long implicitTransactionTimeout = 3_000;
+ public final long minRoTimeout = 1;
+
+ @Range(min = 1)
+ @Value(hasDefault = true)
+ public final long maxRoTimeout = Long.MAX_VALUE;
Review Comment:
Here and there: for such properties it's reasonable to have milis(or any
other) suffix.
--
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]
