-
Notifications
You must be signed in to change notification settings - Fork 102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IGNITE-24242 Add RO/RW Transaction Timeout configuration schema #5093
Changes from 20 commits
68f1fa8
2e7c24c
c4fcc59
03a0274
7dcb643
1e21f92
a34ded1
9b6203a
17f37e4
4340ec3
db2fcef
71ac52d
ba756bc
c05476d
fa7f5df
cc9cb48
ae00d30
e1ee039
2f1e409
c0b0e40
2f13beb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still looks like a breaking change. Let's consult @ptupitsyn whether we are allowed to directly remove a configuration property There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. But why should we consult Pavel about changing the property we've added a couple of weeks ago? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because 3.0.0 already contains |
||
public final long readOnlyTimeout = TimeUnit.MINUTES.toMillis(10); | ||
|
||
/** Timeout for implicit transactions (milliseconds). */ | ||
/** Default timeout for read-write transactions. */ | ||
@Range(min = 1) | ||
@Value(hasDefault = true) | ||
public final long implicitTransactionTimeout = 3_000; | ||
public final long readWriteTimeout = TimeUnit.SECONDS.toMillis(30); | ||
|
||
/** A transaction tries to take lock several times until it throws an exception {@lonk org.apache.ignite.tx.TransactionException}. */ | ||
@Range(min = 0) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -416,7 +416,11 @@ private InternalTransaction beginBusy( | |
if (!readOnly) { | ||
txStateVolatileStorage.initialize(txId, localNodeId); | ||
|
||
return new ReadWriteTransactionImpl(this, timestampTracker, txId, localNodeId, implicit); | ||
// 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; | ||
|
||
return new ReadWriteTransactionImpl(this, timestampTracker, txId, localNodeId, implicit, timeout); | ||
} else { | ||
return beginReadOnlyTransaction(timestampTracker, beginTimestamp, txId, implicit, options); | ||
} | ||
|
@@ -447,7 +451,11 @@ private ReadOnlyTransactionImpl beginReadOnlyTransaction( | |
try { | ||
CompletableFuture<Void> txFuture = new CompletableFuture<>(); | ||
|
||
var transaction = new ReadOnlyTransactionImpl(this, timestampTracker, txId, localNodeId, implicit, readTimestamp, txFuture); | ||
long timeout = options.timeoutMillis() == 0 ? defaultReadOnlyTransactionTimeoutMillis() : options.timeoutMillis(); | ||
|
||
var transaction = new ReadOnlyTransactionImpl( | ||
this, timestampTracker, txId, localNodeId, implicit, timeout, readTimestamp, txFuture | ||
); | ||
|
||
// Implicit transactions are finished as soon as their operation/query is finished, they cannot be abandoned, so there is | ||
// no need to register them. | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This calculation is repeated at least twice. Let's extract it to a method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The next ticket will change this, don't you mind to leave this as is? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok with this if this will not be forgotten :) |
||
return sumWithSaturation(beginTimestamp.getPhysical(), effectiveTimeoutMillis); | ||
} | ||
|
||
|
@@ -493,8 +501,8 @@ private static long sumWithSaturation(long a, long b) { | |
} | ||
} | ||
|
||
private long defaultTransactionTimeoutMillis() { | ||
return txConfig.timeout().value(); | ||
private long defaultReadOnlyTransactionTimeoutMillis() { | ||
return txConfig.readOnlyTimeout().value(); | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10_000 is used for the same purpose in multiple places. Should we extract it somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next ticket will change this, don't you mind to leave this as is?