IGNITE-27947 Impossible to rollback client first transaction request#7730
IGNITE-27947 Impossible to rollback client first transaction request#7730tmgodinho wants to merge 2 commits intoapache:mainfrom
Conversation
** Code should be very similar to ClientTable
…BlockOnLockConflictWithDirectMapping to also test SQL CLient
| // 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); |
There was a problem hiding this comment.
Could you check what the appropriate ID is for the transaction in this case?
I could not reason about it.
There was a problem hiding this comment.
Pull request overview
Fixes the client-side rollback behavior when a lazily-started transaction’s first request fails (IGNITE-27947), and expands the integration test coverage to exercise both KV and SQL paths.
Changes:
- Complete
WriteContext.firstReqFuton SQL request failure by creating a failedClientTransaction, mirroring existing table/KV behavior. - Re-enable and convert the previously disabled rollback/lock-conflict test to a parameterized test that runs for both KV and SQL operations.
- Adjust the test to initialize SQL partition-awareness metadata before running the lock-conflict scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientTransactionsTest.java | Reworks and broadens the rollback-on-first-request test to run for KV + SQL and updates setup/cleanup accordingly. |
| modules/client/src/main/java/org/apache/ignite/internal/client/sql/ClientSql.java | Adds error-path logic to complete firstReqFut with a failed transaction so rollback doesn’t hang after a failed first request. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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); | ||
| } |
There was a problem hiding this 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').
| // Wait for the future to finish, not really necessary. Could just wait a bit. | ||
| await().atMost(1, TimeUnit.SECONDS).until(fut2::isDone); |
There was a problem hiding this 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).
| // Wait for the future to finish, not really necessary. Could just wait a bit. | |
| await().atMost(1, TimeUnit.SECONDS).until(fut2::isDone); | |
| // Future should still be waiting on the lock at this point. | |
| assertFalse(fut2.isDone()); |
| public void testRollbackDoesNotBlockOnLockConflictDuringFirstRequest(KillTestContext ctx) { | ||
| // Note: reversed tx priority is required for this test. | ||
| ClientTable table = (ClientTable) table(); | ||
| ClientSql sql = (ClientSql) client().sql(); |
There was a problem hiding this comment.
According to the note at line 1406 [1], you need to enable the reversed tx priority for this test.
This is required to ensure tx2 (younger) waits for locks from tx1 (older).
After switching to WOUND_WAIT [2] this will be the default behavior.
Otherwise, the test doesn't reproduce anything.
[1] https://github.com/apache/ignite-3/pull/7730/changes#diff-6764538010ba679d53a71a8c6f2251eda1fd68e73cc1ec932a7ddeefcc573fafL1407
[2] https://issues.apache.org/jira/browse/IGNITE-24963
There was a problem hiding this comment.
To enable reversed priotity, you need to change default comparator value:
private static final TxIdComparators DEFAULT_TX_ID_COMPARATOR = TxIdComparators.REVERSED;
https://issues.apache.org/jira/browse/IGNITE-27947
What was done:
Issues: