Copilot commented on code in PR #7799:
URL: https://github.com/apache/ignite-3/pull/7799#discussion_r3039081194
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -552,17 +628,16 @@ public CompletableFuture<Lock> acquire(UUID txId,
LockMode lockMode) {
assert false : "Should not reach here";
}
- // Validate reordering with IX locks if prevention
is enabled.
- if (deadlockPreventionPolicy.usePriority()) {
- for (Lock lock : ixlockOwners.values()) {
- // Allow only high priority transactions
to wait.
- if (txComparator.compare(lock.txId(),
txId) < 0) {
- return notifyAndFail(txId,
lock.txId(), lockMode, lock.lockMode());
- }
+ // Prevent deadlocks by allowing only younger
transactions to wait.
+ for (Lock lock : ixlockOwners.values()) {
+ if
(deadlockPreventionPolicy.txIdComparator().compare(txId, lock.txId()) < 0) {
+ return notifyAndFail(txId, lock.txId(),
lockMode, lock.lockMode());
}
Review Comment:
In coarse-lock S acquisition, `deadlockPreventionPolicy.txIdComparator()` is
dereferenced without a null-check. Policies like
`NoWaitDeadlockPreventionPolicy` / `TimeoutDeadlockPreventionPolicy` currently
return `null`, which will cause an immediate NPE here. Consider either
requiring a non-null comparator in `DeadlockPreventionPolicy` (return a
default) or using a local fallback comparator (e.g., `UUID::compareTo`) at this
call site.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/WoundWaitDeadlockPreventionPolicy.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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;
+
+/**
+ * Implements a deadlock prevention policy that resolves conflicts between two
transactions (tx1 and tx2) contending for the same key. When
+ * tx1 holds a lock and tx2 attempts to acquire it, the policy allows tx2 to
wait for the lock if any of the following conditions are
+ * met:
+ * <ul>
+ * <li>tx2 is younger than tx1.</li>
+ * <li>tx2 is older than tx1 but has a lower {@link
org.apache.ignite.internal.tx.TxPriority}.</li>
+ * <li>The wait timeout is greater than 0.</li>
+ * </ul>
+ * If none of these conditions are met, tx1 is killed to prevent deadlock.
+ */
Review Comment:
The class-level Javadoc lists additional conditions (e.g., “wait timeout >
0”) that affect whether the waiter is allowed to wait, but `allowWait`
currently depends only on the comparator result (waiter younger => wait;
otherwise wound owner). Please align the documentation with the actual
behavior, or incorporate the documented conditions into the implementation.
##########
modules/api/src/main/java/org/apache/ignite/tx/RunInTransactionInternalImpl.java:
##########
@@ -311,10 +282,7 @@ private static CompletableFuture<Void>
throwExceptionWithSuppressedAsync(Throwab
}
private static boolean isRetriable(Throwable e) {
- return hasCause(e,
- TimeoutException.class,
- RetriableTransactionException.class
- );
+ return hasCause(e, RetriableTransactionException.class);
}
Review Comment:
`isRetriable` now only checks for `RetriableTransactionException`. However,
the public `IgniteTransactions#runInTransaction*` Javadoc still explicitly
lists `TimeoutException` as a retried/expected exception. Either ensure
timeout-related failures are wrapped/marked as `RetriableTransactionException`,
or update the API documentation to match the new behavior.
##########
modules/transactions/src/testFixtures/java/org/apache/ignite/internal/tx/test/LockWaiterMatcher.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.test;
+
+import java.util.UUID;
+import java.util.concurrent.CancellationException;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import org.apache.ignite.internal.tx.Lock;
+import org.hamcrest.Description;
+import org.hamcrest.TypeSafeMatcher;
+
+/**
+ * Validates if a lock future will wait for expected owner.
+ */
+public class LockWaiterMatcher extends
TypeSafeMatcher<CompletableFuture<Lock>> {
+ private final UUID waiterId;
+
+ private LockWaiterMatcher(UUID txId) {
+ this.waiterId = txId;
+ }
+
+ @Override
+ protected boolean matchesSafely(CompletableFuture<Lock> item) {
+ try {
+ item.get(50, TimeUnit.MILLISECONDS);
+ return false; // Timeout exception is expected.
+ } catch (TimeoutException e) {
+ return true;
+ } catch (InterruptedException | ExecutionException |
CancellationException e) {
+ throw new AssertionError(e);
+ }
+ }
+
+ @Override
+ protected void describeMismatchSafely(CompletableFuture<Lock> item,
Description mismatchDescription) {
+ mismatchDescription.appendText("lock future is completed
").appendValue(item);
+ }
+
+ @Override
+ public void describeTo(Description description) {
+ description.appendText("lock future which should wait for
").appendValue(waiterId);
+ }
+
+ public static LockWaiterMatcher waitsFor(UUID... txIds) {
+ return new LockWaiterMatcher(txIds[0]);
+ }
Review Comment:
`LockWaiterMatcher` stores `waiterId` and exposes `waitsFor(UUID... txIds)`,
but `matchesSafely` only checks that the future does *not* complete within 50ms
and never uses `waiterId` (or any of the provided IDs). This makes the matcher
name/description misleading and can let incorrect owner/wait relationships
pass. Either verify the expected owner (if possible) or drop the unused
ID/varargs and rename to reflect that it only asserts “not completed yet”.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/InternalTransaction.java:
##########
@@ -190,4 +190,8 @@ CompletableFuture<Void> finish(
default void processDelayedAck(Object val, @Nullable Throwable err) {
// No-op.
}
+
+ default RuntimeException enlistFailedException() {
+ return null;
Review Comment:
`InternalTransaction.enlistFailedException()` returns `null` by default, but
callers (e.g., `InternalTableImpl`) pass it to
`CompletableFuture.failedFuture(...)`, which will NPE on a null throwable. This
default should return a non-null exception (or throw) so implementations that
don't override it fail deterministically.
```suggestion
return new IllegalStateException("Transaction implementation must
provide a non-null enlist failure exception");
```
##########
modules/client/src/integrationTest/java/org/apache/ignite/internal/streamer/ItClientDataStreamerLoadTest.java:
##########
@@ -130,13 +160,20 @@ private static void streamData(IgniteClient client) {
// Insert same data over and over again.
for (int j = 0; j < LOOP_COUNT; j++) {
+ LOG.info("Loop " + j);
for (int i = 0; i < ROW_COUNT; i++) {
publisher.submit(DataStreamerItem.of(tuple(i, "foo_" +
i)));
}
}
}
- streamerFut.orTimeout(10, TimeUnit.SECONDS).join();
+ try {
+ streamerFut.orTimeout(10, TimeUnit.SECONDS).join();
+ LOG.info("Done streaming");
+ } catch (Exception e) {
+ // Don't expecting errors here with proper retries TODO
https://issues.apache.org/jira/browse/IGNITE-28365
+ LOG.warn("Done streaming with error", e);
Review Comment:
This test helper swallows any exception from
`streamerFut.orTimeout(...).join()` and only logs a warning. That can let
`testHighLoad` pass (or fail later for unrelated reasons) even when streaming
fails, reducing the test’s ability to detect regressions. Consider failing the
test on streaming errors (or at least rethrow after logging) once IGNITE-28365
is addressed.
```suggestion
LOG.warn("Done streaming with error", e);
throw new AssertionError("Streaming failed", e);
```
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/LockKey.java:
##########
@@ -88,6 +89,14 @@ public int hashCode() {
@Override
public String toString() {
- return S.toString(LockKey.class, this, "ctx", contextId, "key", key);
+ return S.toString(LockKey.class, this, "ctx", contextId, "key",
dump(key));
+ }
+
+ private static String dump(Object key) {
+ if (key instanceof ByteBuffer) {
+ return Arrays.toString(((ByteBuffer) key).array());
Review Comment:
`LockKey.toString()` can throw at runtime: `((ByteBuffer) key).array()`
fails for direct or read-only buffers (and ignores position/limit). Since
`LockKey` is used with `BinaryTuple.byteBuffer()` (which returns a sliced
buffer), this makes logging/debugging potentially crash. Use a safe dump that
does not call `array()` unless `hasArray()`, and respects `remaining()` (e.g.,
copy bytes from a duplicate buffer).
```suggestion
ByteBuffer buffer = ((ByteBuffer) key).duplicate();
byte[] bytes = new byte[buffer.remaining()];
buffer.get(bytes);
return Arrays.toString(bytes);
```
--
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]
