tmgodinho commented on code in PR #7844:
URL: https://github.com/apache/ignite-3/pull/7844#discussion_r2974376962
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -1200,6 +1225,8 @@ private void processOperationInternal(
writeError(requestId, opCode, e, ctx, false);
metrics.requestsFailedIncrement();
}
+
+ firstReqToTxResMap.remove(requestId);
Review Comment:
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.
--
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]
