You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "Cyrill (via GitHub)" <gi...@apache.org> on 2026/03/27 19:01:44 UTC
[PR] IGNITE-28385 Fix missing enlistment in client commit request handler [ignite-3]
Cyrill opened a new pull request, #7899:
URL: https://github.com/apache/ignite-3/pull/7899
https://issues.apache.org/jira/browse/IGNITE-28385
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28385 Fix missing enlistment in client commit request handler [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #7899:
URL: https://github.com/apache/ignite-3/pull/7899#discussion_r3010526491
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxRecoveryEngine.java:
##########
@@ -127,10 +129,24 @@ public CompletableFuture<TransactionMeta> triggerTxRecovery(
})
.thenCompose(Function.identity())
.whenComplete((v, ex) -> {
- runCleanupOnNode(commitPartitionId, txId, commitPartitionNode);
+ // v contains the actual resolved transaction state (COMMITTED or ABORTED).
+ // We must pass it to cleanup so the correct commit flag is used.
+ // Previously, runCleanupOnNode always used commit=false (hardcoded in the 3-param
+ // TxCleanupRequestSender.cleanup), which caused data corruption when the transaction
+ // was actually COMMITTED: the abort cleanup would race with the legitimate commit
+ // cleanup, and whichever arrived first at each partition would win, producing a mix
+ // of committed and aborted rows within the same transaction.
Review Comment:
I would reword this without "Previously", let's just leave an explanation "why it looks like this right now"
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxManagerImpl.java:
##########
@@ -660,7 +660,7 @@ public CompletableFuture<Void> finish(
Map<ZonePartitionId, PendingTxPartitionEnlistment> enlistedGroups,
UUID txId
) {
- LOG.debug("Finish [commit={}, {}, groups={}, commitPartId={}].", commitIntent,
+ LOG.info("Finish [commit={}, {}, groups={}, commitPartId={}].", commitIntent,
Review Comment:
please change back to `debug`
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/tx/ClientTransactionCommitRequest.java:
##########
@@ -172,8 +172,11 @@ public static boolean merge(InternalTable table, int partId, String consistentId
if (existing == null) {
tx.enlist(replicationGroupId, table.tableId(), consistentId, token);
} else {
+ boolean tokenMatch = existing.consistencyToken() == token;
+ existing.addTableId(table.tableId());
+
// Enlistment tokens should be equal on commit.
- return !commit || existing.consistencyToken() == token;
+ return !commit || tokenMatch;
Review Comment:
on commit, this will always return false and lead to exception, does it really work?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxManagerImpl.java:
##########
@@ -906,7 +906,7 @@ private CompletableFuture<Void> sendFinishRequest(
HybridTimestamp commitTimestamp,
CompletableFuture<TransactionMeta> txFinishFuture
) {
- LOG.debug("Finish [partition={}, node={}, enlistmentConsistencyToken={}, commit={}, {}, groups={}",
+ LOG.info("Finish [partition={}, node={}, enlistmentConsistencyToken={}, commit={}, {}, groups={}",
Review Comment:
please change back to `debug`
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28385 Fix missing enlistment in client commit request handler [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #7899:
URL: https://github.com/apache/ignite-3/pull/7899#discussion_r3011730792
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/tx/ClientTransactionCommitRequest.java:
##########
@@ -172,8 +172,11 @@ public static boolean merge(InternalTable table, int partId, String consistentId
if (existing == null) {
tx.enlist(replicationGroupId, table.tableId(), consistentId, token);
} else {
+ boolean tokenMatch = existing.consistencyToken() == token;
+ existing.addTableId(table.tableId());
+
// Enlistment tokens should be equal on commit.
- return !commit || existing.consistencyToken() == token;
+ return !commit || tokenMatch;
Review Comment:
oh shit, there is `||`
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28385 Fix missing enlistment in client commit request handler [ignite-3]
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #7899:
URL: https://github.com/apache/ignite-3/pull/7899#discussion_r3010814607
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxManagerImpl.java:
##########
@@ -660,7 +660,7 @@ public CompletableFuture<Void> finish(
Map<ZonePartitionId, PendingTxPartitionEnlistment> enlistedGroups,
UUID txId
) {
- LOG.debug("Finish [commit={}, {}, groups={}, commitPartId={}].", commitIntent,
+ LOG.info("Finish [commit={}, {}, groups={}, commitPartId={}].", commitIntent,
Review Comment:
fixed
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxManagerImpl.java:
##########
@@ -906,7 +906,7 @@ private CompletableFuture<Void> sendFinishRequest(
HybridTimestamp commitTimestamp,
CompletableFuture<TransactionMeta> txFinishFuture
) {
- LOG.debug("Finish [partition={}, node={}, enlistmentConsistencyToken={}, commit={}, {}, groups={}",
+ LOG.info("Finish [partition={}, node={}, enlistmentConsistencyToken={}, commit={}, {}, groups={}",
Review Comment:
fixed
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28385 Fix missing enlistment in client commit request handler [ignite-3]
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #7899:
URL: https://github.com/apache/ignite-3/pull/7899#discussion_r3010836543
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxRecoveryEngine.java:
##########
@@ -127,10 +129,24 @@ public CompletableFuture<TransactionMeta> triggerTxRecovery(
})
.thenCompose(Function.identity())
.whenComplete((v, ex) -> {
- runCleanupOnNode(commitPartitionId, txId, commitPartitionNode);
+ // v contains the actual resolved transaction state (COMMITTED or ABORTED).
+ // We must pass it to cleanup so the correct commit flag is used.
+ // Previously, runCleanupOnNode always used commit=false (hardcoded in the 3-param
+ // TxCleanupRequestSender.cleanup), which caused data corruption when the transaction
+ // was actually COMMITTED: the abort cleanup would race with the legitimate commit
+ // cleanup, and whichever arrived first at each partition would win, producing a mix
+ // of committed and aborted rows within the same transaction.
Review Comment:
fixed
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28385 Fix missing enlistment in client commit request handler [ignite-3]
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #7899:
URL: https://github.com/apache/ignite-3/pull/7899#discussion_r3010803100
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/tx/ClientTransactionCommitRequest.java:
##########
@@ -172,8 +172,11 @@ public static boolean merge(InternalTable table, int partId, String consistentId
if (existing == null) {
tx.enlist(replicationGroupId, table.tableId(), consistentId, token);
} else {
+ boolean tokenMatch = existing.consistencyToken() == token;
+ existing.addTableId(table.tableId());
+
// Enlistment tokens should be equal on commit.
- return !commit || existing.consistencyToken() == token;
+ return !commit || tokenMatch;
Review Comment:
why?
on commit we have `false || tokensMatch`. In the case of matching tokens we have `false || true` == `true`
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28385 Fix missing enlistment in client commit request handler [ignite-3]
Posted by "ashapkin (via GitHub)" <gi...@apache.org>.
ashapkin merged PR #7899:
URL: https://github.com/apache/ignite-3/pull/7899
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org