IGNITE-27947 Fix rollback for client piggyback tx by tmgodinho · Pull Request #7844 · apache/ignite-3

tmgodinho · 2026-03-20T18:22:57Z

Opened a new commit following #7730 with a much simpler approach.

https://issues.apache.org/jira/browse/IGNITE-27947

What was done:

Implement error handling for unset transaction in ClientSql
Server-side:
** TX_ROLLBACK accpets a request id of the first request of a direct mapped TX. Request Id is encoded in the negative range of resourceId.
** First Request Ids per TX are tracked using a mapping from reqId to resourceId. Mappings are created when the TX is created and removed on rollback and on response sent.
** Update all the operations. RO ops have the same parameters just for consistency.
Client Side:
** Allow multiple onSent callbacks on the payload output object.
** Added information to ClientLazyTransaction about the first request in the TX. Updated via PayloadOutputChannel on successful request.
** Implemented TX_ROLLBACK based on firstReqId

** Code should be very similar to ClientTable

* The first request is tracked for direct transactions. * The first request id is passed on the Rollback message using the negative range of the resourceId field.

…llbacks

…client.

…nsactionsTest

tmgodinho · 2026-03-20T18:23:40Z

This should be a simpler implementation than the one on the previous approach.

ptupitsyn · 2026-03-23T09:23:14Z

...ient-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java

                metrics.requestsFailedIncrement();
            }
+
+            firstReqToTxResMap.remove(requestId);


Here we remove the mapping once the response is sent, but the client might have requested cancellation just before it received our response:

client: send request server: firstReqToTxResMap.add server: send response, `firstReqToTxResMap.remove` client: cancel operation server: returns an error?

Can we remove the firstReqToTxResMap later? For example, when the tx is cleaned up (removed from the resource registry)?

This should simplify client-side logic too, we can always rollback using the first request id.

Hi Pavel,
Yeah, you are right.
The rollback message using the firstReqId may already be waiting to be processed when we remove here.
In this scenario, the server would respond with an error, but we are not interested in that response.
That's why we also attach to the actual transaction future from the server and execute the rollback using the normal method if necessary. If we received the first request response concurrently, we just resend the rollback using the normal way, even if the first rollback was already sent.
But I forgot to add this scenario to the tests. I had it in the first version but forgot to port it to this one.

Delaying the removal of the mapping is also possible, at the expense of slightly more complexity on the server-side. I thought about it during the impl. On the client-side it would probably be simpler as you said, since we would rely on the server response for the first rollback message.

I'll add the test and implement this to see how it looks.

Could you recheck @ptupitsyn ??

** Postpone removal of firstReqToTx resource to after the transaction is commited/rolled back. ** Added test

.../java/org/apache/ignite/client/handler/requests/table/DirectTransactionWithFirstRequest.java

...ient-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java

ptupitsyn · 2026-03-30T13:09:52Z

modules/client/src/main/java/org/apache/ignite/internal/client/sql/ClientSql.java

                    tx.recordOperationFailure(err);
                }

+                // We should reconcile this code with ClientTable. Should be the same.


Can we do that as part of current PR?

Yes, I also think it would be best.

ptupitsyn · 2026-03-30T13:10:31Z

modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientLazyTransaction.java

    public CompletableFuture<Void> rollbackAsync() {
        var tx0 = tx;

+        // This is really fishy. It will probably let you reuse a transaction after calling a rollback :(


Good catch. Let's create a ticket to address this.

@tmgodinho please share the ticket.

ptupitsyn · 2026-03-30T13:11:34Z

modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientLazyTransaction.java

+        if (!tx0.isDone() && cancelled.compareAndSet(false, true)) {
+            return requestInfoFuture
+                    .thenCompose(reqInfo ->
+                            reqInfo.ch.serviceAsync(ClientOp.TX_ROLLBACK, w -> w.out().packLong(-reqInfo.firstReqId), r -> null)


Protocol compat: what will happen with an older server that does not understand negative resource ids?

Essentially, it will experience the old "blocking" behaviour.
The request will be sent to the server, the server will panic at:
https://github.com/apache/ignite-3/pull/7844/changes/BASE..f649f764f18e572f830208127faddbc566fc8a6f#diff-6e5ede08174517c940134dafdcb9270727ed3738dcb475582b48c017344896f2L59
The client will pick the error, and try to rollback using the old method once the tx information is known.

I assumed it was ok just to fix it for newer versions without breaking the previous ones.

PS: The error is something like, resource not found, nothing very relevant.

I think we need a protocol feature flag for this. Avoid sending the request if the server does not understand it.

** Fixed type ** Updated javadoc comment on ClientInboundMessageHandler ** Added IGNITE-28405 ticket mention

…andling logic. (For the most part anyway) ** The code is still very hard, its just a little less duplicated.

# Conflicts: # modules/client-common/src/main/java/org/apache/ignite/internal/client/proto/ProtocolBitmaskFeature.java # modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientHandlerModule.java # modules/client/src/main/java/org/apache/ignite/internal/client/TcpClientChannel.java

ptupitsyn · 2026-04-03T10:08:21Z

modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientLazyTransaction.java

+                        if (ch.protocolContext().isFeatureSupported(TX_ROLLBACK_USING_FIRST_REQUEST)) {
+                            return ch.serviceAsync(ClientOp.TX_ROLLBACK, w -> w.out().packLong(-reqInfo.firstReqId), r -> null);
+                        } else {
+                            return NOT_SUPPORTED_FUTURE;


What does this mean for the user?

ptupitsyn · 2026-04-03T10:08:56Z

modules/client/src/main/java/org/apache/ignite/internal/client/tx/DirectTxUtils.java

    }

+    /**
+     * If the current request is the first request of a direct translation, add a listener to the {@link PayloadWriter}.


Suggested change

* If the current request is the first request of a direct translation, add a listener to the {@link PayloadWriter}.

* If the current request is the first request of a direct tx, add a listener to the {@link PayloadWriter}.

ptupitsyn · 2026-04-03T10:13:25Z

...src/integrationTest/java/org/apache/ignite/internal/client/ItThinClientTransactionsTest.java

-        kvView.put(tx1, key, val);
+        @ParameterizedTest
+        @MethodSource("org.apache.ignite.internal.client.ItThinClientTransactionsTest#killTestContextFactory")
+        public void testRollbackDoesNotBlock(KillTestContext ctx) throws InterruptedException {


All those new tests pass on the main branch, looks like we don't test the new functionality.

tmgodinho added 5 commits March 20, 2026 17:04

IGNITE-27947 Implement error handling for unset transaction in ClientSql

4e91edf

** Code should be very similar to ClientTable

IGNITE-27947 Add TX_Rollback implementation based on firstReqId

74b3741

* The first request is tracked for direct transactions. * The first request id is passed on the Rollback message using the negative range of the resourceId field.

IGNITE-27947 Allow PayloadOutputChannel to support multiple onSent ca…

e7df787

…llbacks

IGNITE-27947 Implement early rollback for Direct Transactions on the …

46e0cd1

…client.

IGNITE-27947 Add tests for Tx Blocked on conflicts in ItThinClientTra…

49c0fc2

…nsactionsTest

tmgodinho marked this pull request as ready for review March 20, 2026 19:21

tmgodinho requested review from isapego and ptupitsyn as code owners March 20, 2026 19:21

ptupitsyn reviewed Mar 23, 2026

View reviewed changes

tmgodinho added 2 commits March 30, 2026 12:14

IGNITE-27947 Update CLientLazyTransaction error handling logic.

cdf9608

IGNITE-27947 Apply code-review

f649f76

** Postpone removal of firstReqToTx resource to after the transaction is commited/rolled back. ** Added test

tmgodinho force-pushed the ignite-27947 branch from 325b634 to f649f76 Compare March 30, 2026 11:33

tmgodinho requested a review from ptupitsyn March 30, 2026 12:23

ptupitsyn reviewed Mar 30, 2026

View reviewed changes

IGNITE-27947 Apply code-reviews

6c2bf2f

** Fixed type ** Updated javadoc comment on ClientInboundMessageHandler ** Added IGNITE-28405 ticket mention

tmgodinho force-pushed the ignite-27947 branch from 2ce3c2e to 22a3f46 Compare March 30, 2026 22:42

IGNITE-27947 Reconciled ClientSql and ClientTable transaction error h…

52b8fe9

…andling logic. (For the most part anyway) ** The code is still very hard, its just a little less duplicated.

tmgodinho force-pushed the ignite-27947 branch from 22a3f46 to 52b8fe9 Compare March 30, 2026 23:53

tmgodinho requested a review from ptupitsyn March 31, 2026 00:38

IGNITE-27947 Add TX_ROLLBACK_USING_FIRST_REQUEST protocol feature

dfa9bed

tmgodinho force-pushed the ignite-27947 branch from b594952 to dfa9bed Compare April 1, 2026 11:02

ptupitsyn changed the title ~~IGNITE-27947 Impossible to rollback client first transaction request~~ IGNITE-27947 Fix rollback for client piggyback tx Apr 2, 2026

tmgodinho force-pushed the ignite-27947 branch from 72be6a8 to 7b49349 Compare April 2, 2026 15:40

tmgodinho force-pushed the ignite-27947 branch from 7b49349 to 0264715 Compare April 2, 2026 16:56

ptupitsyn reviewed Apr 3, 2026

View reviewed changes

ptupitsyn requested changes Apr 3, 2026

View reviewed changes

	* If the current request is the first request of a direct translation, add a listener to the {@link PayloadWriter}.
	* If the current request is the first request of a direct tx, add a listener to the {@link PayloadWriter}.

Conversation

tmgodinho commented Mar 20, 2026

Uh oh!

tmgodinho commented Mar 20, 2026

Uh oh!

ptupitsyn Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ptupitsyn Mar 23, 2026 •

edited

Loading