Copilot commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2846122311
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/InternalTxOptions.java:
##########
@@ -47,11 +48,16 @@ public class InternalTxOptions {
@Nullable
private final HybridTimestamp readTimestamp;
- private InternalTxOptions(TxPriority priority, long timeoutMillis,
@Nullable HybridTimestamp readTimestamp, @Nullable String txLabel) {
+ /** Transaction kill closure. Defines context specific action on tx kill.
*/
+ private final @Nullable Consumer<InternalTransaction> killClosure;
+
+ private InternalTxOptions(TxPriority priority, long timeoutMillis,
@Nullable HybridTimestamp readTimestamp, @Nullable String txLabel,
+ Consumer<InternalTransaction> killClosure) {
this.priority = priority;
Review Comment:
`killClosure` is treated as nullable (field is `@Nullable` and `defaults()`
builds options without setting it), but the constructor parameter is non-null
`Consumer<InternalTransaction>`. This makes the nullability contract
inconsistent and can confuse callers/static analysis. Consider marking the ctor
parameter (and the Builder field) as `@Nullable` and/or providing an explicit
default no-op closure.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientTransactionsTest.java:
##########
@@ -1183,38 +1342,134 @@ void testReadOnlyTxDoesNotSeeUpdatesAfterStart(boolean
server) {
assertNull(val, "Read-only transaction should not see values committed
after its start");
}
- @Test
- public void testRollbackDoesNotBlockOnLockConflict() {
+ @ParameterizedTest
+ @MethodSource("killTestContextFactory")
+ public void testRollbackDoesNotBlockOnLockConflict(KillTestContext ctx)
throws InterruptedException {
ClientTable table = (ClientTable) table();
- KeyValueView<Tuple, Tuple> kvView = table().keyValueView();
Map<Partition, ClusterNode> map =
table.partitionDistribution().primaryReplicasAsync().join();
- List<Tuple> tuples0 = generateKeysForPartition(800, 10, map, 0, table);
+ List<Tuple> tuples0 = generateKeysForPartition(100, 10, map, 0, table);
ClientLazyTransaction olderTxProxy = (ClientLazyTransaction)
client().transactions().begin();
ClientLazyTransaction youngerTxProxy = (ClientLazyTransaction)
client().transactions().begin();
Tuple key = tuples0.get(0);
Tuple key2 = tuples0.get(1);
- Tuple val = val("1");
- Tuple val2 = val("2");
- kvView.put(olderTxProxy, key, val);
+ assertThat(ctx.put.apply(client(), olderTxProxy, key),
willSucceedFast());
ClientTransaction olderTx = olderTxProxy.startedTx();
- kvView.put(youngerTxProxy, key2, val2);
+ assertThat(ctx.put.apply(client(), youngerTxProxy, key2),
willSucceedFast());
ClientTransaction youngerTx = youngerTxProxy.startedTx();
assertTrue(olderTx.txId().compareTo(youngerTx.txId()) < 0);
// Older is allowed to wait with wait-die.
- CompletableFuture<Void> fut = kvView.putAsync(olderTxProxy, key2, val);
+ CompletableFuture<?> fut = ctx.put.apply(client(), olderTxProxy, key2);
assertFalse(fut.isDone());
+ // Give some time to acquire a lock to avoid a race with next rollback.
+ Thread.sleep(500);
+
Review Comment:
Using `Thread.sleep(500)` as a synchronization aid makes this integration
test timing-dependent and potentially flaky (slow/loaded CI, different
schedulers). Prefer awaiting a specific condition (e.g., using Awaitility on a
predicate that confirms the lock conflict/wait is established) instead of
sleeping a fixed duration.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -206,6 +225,79 @@ public void commit() throws TransactionException {
sync(commitAsync());
}
+ /**
+ * Discards the directly mapped transaction fragments in case of
coordinator side transaction invalidation
+ * (either kill or implicit rollback due to mapping failure, see
postEnlist).
+ *
+ * @param killed Killed flag.
+ *
+ * @return The future.
+ */
+ public CompletableFuture<Void> discardDirectMappings(boolean killed) {
+ enlistPartitionLock.writeLock().lock();
+
+ try {
+ if (!finishFut.compareAndSet(null, new CompletableFuture<>())) {
+ return finishFut.get();
+ }
+ } finally {
+ enlistPartitionLock.writeLock().unlock();
+ }
+
+ return sendDiscardRequests().handle((r, e) -> {
+ setState(killed ? STATE_KILLED : STATE_ROLLED_BACK);
+ ch.inflights().erase(txId());
+ this.finishFut.get().complete(null);
+ return null;
+ });
+ }
+
+ private CompletableFuture<Void> sendDiscardRequests() {
+ assert finishFut != null;
Review Comment:
The `assert finishFut != null;` in `sendDiscardRequests()` is ineffective
because `finishFut` is an `AtomicReference` field that is always non-null. If
the intent is to assert that the transaction is already in finishing state,
assert `finishFut.get() != null` (or drop the assert).
```suggestion
assert finishFut.get() != null;
```
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransactionKilledException.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.client.tx;
+
+import java.util.UUID;
+import org.apache.ignite.tx.TransactionException;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Reports a killed transaction.
+ */
+public class ClientTransactionKilledException extends TransactionException {
+ /** Serial version uid. */
+ private static final long serialVersionUID = 0L;
+
+ /** Transaction id. */
+ private final UUID txId;
+
+ /**
+ * Constructor.
+ *
+ * @param traceId Trace ID.
+ * @param code Error code.
+ * @param message String message.
+ * @param txId Related transaction id.
+ * @param cause The cause.
+ */
+ public ClientTransactionKilledException(UUID traceId, int code, @Nullable
String message, UUID txId, @Nullable Throwable cause) {
+ super(traceId, code, message, cause);
+
+ this.txId = txId;
+ }
+
+ /**
+ * Constructor (for copying purposes).
+ *
+ * @param traceId Trace ID.
+ * @param code Error code.
+ * @param message String message.
+ * @param cause The cause.
+ */
+ public ClientTransactionKilledException(UUID traceId, int code, @Nullable
String message, @Nullable Throwable cause) {
+ super(traceId, code, message, cause);
+
+ this.txId = cause instanceof ClientTransactionKilledException
+ ? ((ClientTransactionKilledException) cause).txId : null;
+ }
+
+ /**
+ * Returns a related transaction id.
+ *
+ * @return The id.
+ */
+ public UUID txId() {
+ return txId;
+ }
Review Comment:
`txId` can become `null` in the copy constructor (when `cause` is not a
`ClientTransactionKilledException`), but the field and accessor are non-null
(`UUID`) and callers (e.g., notification handling) assume a non-null id.
Consider making `txId`/`txId()` `@Nullable` or enforcing a non-null `txId` in
all constructors (e.g., require/derive it deterministically) to avoid potential
NPEs and clarify the contract.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -383,35 +499,34 @@ public CompletableFuture<IgniteBiTuple<String, Long>>
enlistFuture(ReliableChann
throw new TransactionException(TX_ALREADY_FINISHED_ERR,
format("Transaction is already finished [tx={}].", this));
Review Comment:
In `enlistFuture`, when `tryLock()` fails you always throw
`TX_ALREADY_FINISHED_ERR`. This can misreport the error for killed transactions
(state `STATE_KILLED`) since you already have `exceptionForState(...)` to
return `TX_KILLED_ERR`. Consider checking the current state/`finishFut` and
throwing `exceptionForState(state.get(), this)` (or otherwise preserving the
killed vs finished distinction) on the lock-acquisition failure path.
```suggestion
throw exceptionForState(state.get(), this);
```
--
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]
