Copilot commented on code in PR #7730:
URL: https://github.com/apache/ignite-3/pull/7730#discussion_r2911405771
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientTransactionsTest.java:
##########
@@ -1401,34 +1400,53 @@ public void
testRollbackDoesNotBlockOnLockConflict(KillTestContext ctx) {
assertThat(kvView.removeAllAsync(null, Arrays.asList(key0, key,
key2)), willSucceedFast());
}
- @Test
- @Disabled("https://issues.apache.org/jira/browse/IGNITE-27947";)
- public void testRollbackDoesNotBlockOnLockConflictDuringFirstRequest()
throws InterruptedException {
+ @ParameterizedTest
+ @MethodSource("killTestContextFactory")
+ public void
testRollbackDoesNotBlockOnLockConflictDuringFirstRequest(KillTestContext ctx) {
// Note: reversed tx priority is required for this test.
ClientTable table = (ClientTable) table();
+ ClientSql sql = (ClientSql) client().sql();
KeyValueView<Tuple, Tuple> kvView = table().keyValueView();
Map<Partition, ClusterNode> map =
table.partitionDistribution().primaryReplicasAsync().join();
- List<Tuple> tuples0 = generateKeysForPartition(100, 10, map, 0, table);
+ Entry<Partition, ClusterNode> mapping =
map.entrySet().iterator().next();
+ List<Tuple> tuples0 = generateKeysForPartition(100, 10, map, (int)
mapping.getKey().id(), table);
+ Ignite server = server(mapping.getValue());
+ IgniteImpl ignite = unwrapIgniteImpl(server);
+
+ // Init SQL mappings.
+ Tuple key0 = tuples0.get(0);
+ sql.execute(format("INSERT INTO %s (%s, %s) VALUES (?, ?)",
TABLE_NAME, COLUMN_KEY, COLUMN_VAL),
+ key0.intValue(0), key0.intValue(0) + "");
+ await().atMost(2, TimeUnit.SECONDS)
+ .until(() ->
sql.partitionAwarenessCachedMetas().stream().allMatch(PartitionMappingProvider::ready));
// We need a waiter for this scenario.
- Tuple key = tuples0.get(0);
- Tuple val = val("1");
+ Tuple key = tuples0.get(1);
ClientLazyTransaction tx1 = (ClientLazyTransaction)
client().transactions().begin();
ClientLazyTransaction tx2 = (ClientLazyTransaction)
client().transactions().begin();
- kvView.put(tx1, key, val);
+ // Starts the transaction.
+ assertThat(ctx.put.apply(client(), tx1, key), willSucceedFast());
- // Will wait for lock.
- CompletableFuture<Void> fut2 = kvView.putAsync(tx2, key, val);
- assertFalse(fut2.isDone());
+ await().atMost(3, TimeUnit.SECONDS).until(() -> {
+ Iterator<Lock> locks =
ignite.txManager().lockManager().locks(tx1.startedTx().txId());
- Thread.sleep(500);
+ int count = CollectionUtils.count(locks);
+ return count == 2;
+ });
+
+ // Will wait for lock.
+ CompletableFuture<?> fut2 = ctx.put.apply(client(), tx2, key);
+ // Wait for the future to finish, not really necessary. Could just
wait a bit.
+ await().atMost(1, TimeUnit.SECONDS).until(fut2::isDone);
Review Comment:
This test is named and commented as if the second operation should block on
a lock ("Will wait for lock"), but it then explicitly waits for fut2 to
complete within 1 second and never asserts that fut2 actually failed with the
expected lock-conflict error. This can make the test both misleading and weaker
(it may pass even if no conflict happens). Consider either asserting fut2 is
not done before rollback (if the intent is to have a waiter) or asserting fut2
completes exceptionally with ctx.expectedErr (and avoid the fixed 1s await by
using the existing willThrowWithCauseOrSuppressed matcher).
```suggestion
// Future should still be waiting on the lock at this point.
assertFalse(fut2.isDone());
```
##########
modules/client/src/main/java/org/apache/ignite/internal/client/sql/ClientSql.java:
##########
@@ -373,6 +373,18 @@ public <T> CompletableFuture<AsyncResultSet<T>>
executeAsyncInternal(
false
).handle((BiFunction<AsyncResultSet<T>, Throwable,
CompletableFuture<AsyncResultSet<T>>>) (r, err) -> {
if (err != null) {
+ if (ctx.firstReqFut != null) {
+ // Create failed transaction.
+ // TODO: Check what is the id here.
+ long id = -1;
+ ClientTransaction failed = new
ClientTransaction(ctx.channel, ch, id, ctx.readOnly, null,
+ ctx.pm, null, ch.observableTimestamp(), 0);
+ failed.fail();
+ ctx.firstReqFut.complete(failed);
+ // Txn was not started, rollback is not required.
+ return failedFuture(err);
+ }
Review Comment:
New error-path logic completes ctx.firstReqFut by constructing a
ClientTransaction with a placeholder id (-1) and a TODO. This leaves production
code with an unresolved TODO and a magic value; if the id ever appears in
logs/toString or future logic starts using it, behavior becomes ambiguous.
Consider removing the TODO and using a clearly documented sentinel constant
(e.g., 0L) or refactoring so firstReqFut is completed without needing a
synthetic transaction id (for example, by completing it exceptionally and
making ClientLazyTransaction.rollbackAsync treat that as 'not started').
--
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]
