denis-chudov commented on code in PR #2078:
URL: https://github.com/apache/ignite-3/pull/2078#discussion_r1198822068
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/leases/LeaseTracker.java:
##########
@@ -152,4 +195,46 @@ public CompletableFuture<Void> onUpdate(WatchEvent event) {
public void onError(Throwable e) {
}
}
+
+ @Override
+ public CompletableFuture<LeaseMeta> awaitPrimaryReplica(ReplicationGroupId
groupId, HybridTimestamp timestamp) {
+ if (!busyLock.enterBusy()) {
+ return failedFuture(new NodeStoppingException("Component is
stopping."));
+ }
+ try {
+ return primaryReplicaWaiters.computeIfAbsent(groupId, id -> new
PendingIndependentComparableValuesTracker<>(MIN_VALUE))
+ .waitFor(timestamp);
+ } finally {
+ busyLock.leaveBusy();
+ }
+ }
+
+ @Override
+ public CompletableFuture<LeaseMeta> getPrimaryReplica(ReplicationGroupId
replicationGroupId, HybridTimestamp timestamp) {
+ if (!busyLock.enterBusy()) {
+ return failedFuture(new NodeStoppingException("Component is
stopping."));
+ }
+ try {
+ // There's no sense in awaiting previously detected primary
replica more than lease interval.
+ return awaitPrimaryReplica(replicationGroupId,
timestamp).orTimeout(longLeaseInterval, TimeUnit.MILLISECONDS);
Review Comment:
Consider the following example:
- longLeaseInterval is 2 min
- cluster is experiencing some problems
- user runs very important transaction with a timeout 10 min, they are aware
that there are some problems but have nothing to do with it right now, that's
why the transaction timeout is rather long
- transaction starts, successfully performs some operation within, let say,
1 minute, and is going to be committed with commit timestamp 1000
- commit operation hangs for 2 minutes and then the transaction is rolled
back at timestamp 1120 (let max clock skew be 0), because it was unable to get
lease for timestamp 1000 withing this time interval. Actually lease for the
interval (500, 1002) was received on corresponding node at local time 1121, but
the transaction is already rolled back at this time.
So we have the transaction with timeout 10 minutes that was committed by
user but rolled back by cluster after ~3 minutes from the moment it started. It
is actually implicit commit timeout, which serves also as implicit and
non-configurable transaction timeout. This looks at least weird from the user's
point of view.
I understand that longLeaseInterval is long enough to count it as a
reasonable time of data propagation, but the consequence is that we can roll
back any transaction if some of its operations exceed "reasonable" time which
is not related to transaction timeout, and therefore, arbitrarily roll back any
transaction before it reaches its timeout. This behavior impacts user
experience, seems to be invalid and shouldn't exist.
So, the transaction timeout instead of safe time seems to be more correct
here. I suggest to add parameter (timeout) for this method and leave TODO to
rework it to safe time.
--
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]
