denis-chudov commented on code in PR #7799:
URL: https://github.com/apache/ignite-3/pull/7799#discussion_r3046358143
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/InternalTxOptions.java:
##########
@@ -135,8 +145,13 @@ public Builder killClosure(Consumer<InternalTransaction>
r) {
return this;
}
+ public Builder retryId(UUID id) {
Review Comment:
never used (not even tested?)
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -57,25 +61,28 @@
import org.apache.ignite.internal.tx.LockMode;
import org.apache.ignite.internal.tx.LockTableOverflowException;
import org.apache.ignite.internal.tx.PossibleDeadlockOnLockAcquireException;
+import org.apache.ignite.internal.tx.TransactionIds;
+import org.apache.ignite.internal.tx.TxStateMeta;
import org.apache.ignite.internal.tx.Waiter;
import org.apache.ignite.internal.tx.event.LockEvent;
import org.apache.ignite.internal.tx.event.LockEventParameters;
import org.apache.ignite.internal.util.CollectionUtils;
import org.apache.ignite.internal.util.IgniteStripedReadWriteLock;
+import org.apache.ignite.tx.TransactionException;
import org.jetbrains.annotations.Nullable;
import org.jetbrains.annotations.TestOnly;
/**
* A {@link LockManager} implementation which stores lock queues in the heap.
*
- * <p>Lock waiters are placed in the queue, ordered according to comparator
provided by {@link HeapLockManager#deadlockPreventionPolicy}.
- * When a new waiter is placed in the queue, it's validated against current
lock owner: if there is an owner with a higher priority (as
- * defined by comparator) lock request is denied.
+ * <p>Lock waiters are placed in the queue, ordered according to transaction
priority: older transactions are first.
Review Comment:
`DeadlockPreventionPolicy#txIdComparator` is still used, is it true about
older transactions?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReversedWaitDieDeadlockPreventionPolicy.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.tx.impl;
+
+import java.util.Comparator;
+import java.util.UUID;
+import org.apache.ignite.internal.tx.DeadlockPreventionPolicy;
+import org.apache.ignite.internal.tx.Waiter;
+
+/**
+ * Reversed wait die implementation. Same as wait die, but reverses the wait
order: younger is allowed to wait for older, older is rejected
+ * if conflicts with younger.
+ */
+public class ReversedWaitDieDeadlockPreventionPolicy implements
DeadlockPreventionPolicy {
+ private static final TxIdPriorityComparator TX_ID_PRIORITY_COMPARATOR =
new TxIdPriorityComparator();
+
+ /** {@inheritDoc} */
+ @Override
+ public final Comparator<UUID> txIdComparator() {
+ return TX_ID_PRIORITY_COMPARATOR;
+ }
+
+ @Override
+ public Waiter allowWait(Waiter waiter, Waiter owner) {
+ int res = txIdComparator().compare(waiter.txId(), owner.txId());
+ assert res != 0;
+
+ // Waiter is allowed to wait for owner if it's younger.
+ // Otherwise we have to fail waiter.
+ return res > 0 ? null : waiter;
Review Comment:
What is the reason of defining the same comparator for all policies but
manipulating the comparison result in different ways? The primary goal of
comparator was to avoid this.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/DeadlockPreventionPolicy.java:
##########
@@ -56,11 +59,32 @@ default long waitTimeout() {
}
/**
- * Whether transaction priority if used for conflict resolution.
+ * Invokes fail action on the owner.
+ *
+ * @param owner The owner.
+ */
+ default void failAction(UUID owner) {
+ // No-op.
+ }
+
+ /**
+ * Tests if waiter is allowed to wait for owner.
+ *
+ * @param waiter The waiter.
+ * @param owner The owner.
+ *
+ * @return Waiter to fail or {@code null} if waiting is allowed.
+ */
+ default @Nullable Waiter allowWait(Waiter waiter, Waiter owner) {
+ return null;
+ }
+
+ /**
+ * Returns the order, in which the first conflicting waiter is searched.
*
- * @return Whether priority is used.
+ * @return If {@code true}, searches for older first.
*/
- default boolean usePriority() {
- return txIdComparator() != null;
+ default boolean reverse() {
Review Comment:
This property is directly related to tx comparator. You can define different
comparators (straight and reverse) for different policies, add `reverse`
property to them and make this property refer to comparator's. This would
simplify the logic.
##########
modules/replicator/src/main/java/org/apache/ignite/internal/replicator/exception/ReplicationException.java:
##########
@@ -23,13 +23,11 @@
import org.apache.ignite.internal.lang.IgniteInternalException;
import org.apache.ignite.internal.replicator.ReplicationGroupId;
import org.apache.ignite.tx.RetriableReplicaRequestException;
-import org.apache.ignite.tx.RetriableTransactionException;
/**
* The exception is thrown when some issue happened during a replication.
*/
-public class ReplicationException extends IgniteInternalException implements
RetriableTransactionException,
- RetriableReplicaRequestException {
+public class ReplicationException extends IgniteInternalException implements
RetriableReplicaRequestException {
Review Comment:
I am not sure that removing `RetriableTransactionException` from here is
correct (same is about `ReplicaUnavailableException`). Transactions may be
retried within timeout after failures caused by internal recoverable errors.
Note that some other exceptions are derived from this.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -304,7 +306,8 @@ public class PartitionReplicaListener implements
ReplicaTableProcessor {
private final Supplier<Int2ObjectMap<IndexLocker>> indexesLockers;
- private final ConcurrentMap<UUID, TxCleanupReadyState>
txCleanupReadyFutures = new ConcurrentHashMap<>();
Review Comment:
TxCleanupReadyState is never used now
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -104,14 +111,18 @@ public class HeapLockManager extends
AbstractEventProducer<LockEvent, LockEventP
private Executor delayedExecutor;
/** Enlisted transactions. */
- private final ConcurrentHashMap<UUID, ConcurrentLinkedQueue<Releasable>>
txMap = new ConcurrentHashMap<>(1024);
+ private final ConcurrentHashMap<UUID, SealableQueue> txMap = new
ConcurrentHashMap<>(1024);
/** Coarse locks. */
private final ConcurrentHashMap<Object, CoarseLockState> coarseMap = new
ConcurrentHashMap<>();
/** Tx state required to present tx labels in logs and exceptions. */
private final VolatileTxStateMetaStorage txStateVolatileStorage;
+ private static class SealableQueue extends
ConcurrentLinkedQueue<Releasable> {
+ boolean sealed;
Review Comment:
Please add javadoc about what `sealed` means.
--
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]
