You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ascherbakoff (via GitHub)" <gi...@apache.org> on 2026/02/13 11:07:37 UTC
[PR] IGNITE-27838 Directly mapped transactions not rolled back properly on enlistment failure [ignite-3]
ascherbakoff opened a new pull request, #7588:
URL: https://github.com/apache/ignite-3/pull/7588
(no comment)
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2851206192
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientLazyTransaction.java:
##########
@@ -92,14 +93,7 @@ public CompletableFuture<Void> commitAsync() {
@Override
public void rollback() throws TransactionException {
- var tx0 = tx;
-
- if (tx0 == null) {
- // No operations were performed, nothing to rollback.
- return;
- }
-
- tx0.join().rollback();
+ rollbackAsync().join();
Review Comment:
Wrapped commit and rollback future in ViewUtils.sync. This should be enough.
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2846586407
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/tx/ClientTransactionBeginRequest.java:
##########
@@ -63,6 +68,14 @@ public static CompletableFuture<ResponseWriter> process(
InternalTxOptions txOptions = InternalTxOptions.builder()
.timeoutMillis(timeoutMillis)
.readTimestamp(observableTs)
+ .killClosure(tx -> {
+ if (notificationSender != null) {
+ // Exception will be ignored if a client doesn't support it.
+ TransactionKilledException err = new TransactionKilledException(tx.id(), txManager);
+
+ notificationSender.sendNotification(w -> {}, err, NULL_HYBRID_TIMESTAMP);
+ }
Review Comment:
```suggestion
.killClosure((notificationSender == null)
? tx -> {} // Exception will be ignored if a client doesn't support it.
: tx -> {
TransactionKilledException err = new TransactionKilledException(tx.id(), txManager);
notificationSender.sendNotification(w -> {}, err, NULL_HYBRID_TIMESTAMP);
```
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2847561998
##########
modules/api/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -468,6 +468,9 @@ public static class Transactions {
/** Operation failed due to replication delayed ack failure. */
public static final int TX_DELAYED_ACK_ERR = TX_ERR_GROUP.registerErrorCode((short) 17);
+
+ /** Transaction was internally killed. This is retriable state. */
Review Comment:
What means `retriable`? I don't see it in any "retriable errors" list. Retriable for implicit txns?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxCleanupRequestHandler.java:
##########
@@ -291,6 +291,36 @@ private void sendCleanupReplicatedResponse(UUID txId, InternalClusterNode sender
messagingService.send(sender, ChannelType.DEFAULT, prepareResponse(new CleanupReplicatedInfo(txId, partitions)));
}
+ /**
+ * Discards local write intents.
+ *
+ * @param partitions Partitions.
+ * @param txId The transaction id.
+ *
+ * @return The future.
+ */
+ CompletableFuture<Void> discardLocalWriteIntents(List<EnlistedPartitionGroup> partitions, UUID txId) {
+ Map<EnlistedPartitionGroup, CompletableFuture<?>> writeIntentSwitches = new HashMap<>();
+
+ for (EnlistedPartitionGroup partition : partitions) {
+ CompletableFuture<Void> future = writeIntentSwitchProcessor.switchLocalWriteIntents(
+ partition,
+ txId,
+ false,
+ null
+ ).thenAccept(this::processWriteIntentSwitchResponse);
+
+ writeIntentSwitches.put(partition, future);
+ }
+
+ releaseTxLocks(txId);
Review Comment:
what happens if client dies before sending discard message? afaik the server coordinator doesn't have all partition mappings, who will release the locks?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -239,6 +248,7 @@ private CompletableFuture<Void> finishInternal(
finishFuture = nullCompletedFuture();
this.timeoutExceeded = timeoutExceeded;
} else {
+ assert killClosure == null : "Invalid kill state for a full transaction";
Review Comment:
We have a ticket about avoidance of usage of asserts https://issues.apache.org/jira/browse/IGNITE-27139
Can it be replaced with exception?
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2846115408
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -206,6 +225,79 @@ public void commit() throws TransactionException {
sync(commitAsync());
}
+ /**
+ * Discards the directly mapped transaction fragments in case of coordinator side transaction invalidation
+ * (either kill or implicit rollback due to mapping failure, see postEnlist).
+ *
+ * @param killed Killed flag.
+ *
+ * @return The future.
+ */
+ public CompletableFuture<Void> discardDirectMappings(boolean killed) {
+ enlistPartitionLock.writeLock().lock();
+
+ try {
+ if (!finishFut.compareAndSet(null, new CompletableFuture<>())) {
+ return finishFut.get();
+ }
+ } finally {
+ enlistPartitionLock.writeLock().unlock();
+ }
+
+ return sendDiscardRequests().handle((r, e) -> {
+ setState(killed ? STATE_KILLED : STATE_ROLLED_BACK);
+ ch.inflights().erase(txId());
+ this.finishFut.get().complete(null);
+ return null;
+ });
+ }
+
+ private CompletableFuture<Void> sendDiscardRequests() {
+ assert finishFut != null;
+
+ if (!ch.protocolContext().isFeatureSupported(TX_DIRECT_MAPPING_SEND_DISCARD)) {
+ return nullCompletedFuture();
+ }
+
+ Map<String, List<TablePartitionId>> enlistments = new HashMap<>();
+
+ for (Entry<TablePartitionId, CompletableFuture<IgniteBiTuple<String, Long>>> entry : enlisted.entrySet()) {
+ IgniteBiTuple<String, Long> info = entry.getValue().getNow(null);
+
+ if (info == null) {
+ continue; // Ignore incomplete enlistments.
+ }
+
+ enlistments.computeIfAbsent(info.get1(), k -> new ArrayList<>()).add(entry.getKey());
+ }
+
+ List<CompletableFuture<Void>> futures = new ArrayList<>(enlistments.size());
+
+ for (Entry<String, List<TablePartitionId>> entry : enlistments.entrySet()) {
+ CompletableFuture<Void> discardFut = reliableChannel.getNodeChannelAsync(entry.getKey()).thenCompose(ch -> {
Review Comment:
We should not establish new connections to discard enlistments. There are two ways to approach the problem:
* Enlistment is tied to a client connection => track it on the server in `ClientResourceRegistry` and discard on disconnect
* Enlistment is not tied to a given connection => send **one request to any node** with the list of things to clean up, then the server re-routes as necessary.
We should never assume that the client can connect to a certain node.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -75,7 +89,7 @@ public class ClientTransaction implements Transaction {
/** The future used on repeated commit/rollback. */
@IgniteToStringExclude
- private final AtomicReference<CompletableFuture<Void>> finishFut = new AtomicReference<>();
+ private final AtomicReference<CompletableFuture<Void>> finishFut = new AtomicReference<>(); // TODO use updater
Review Comment:
TODO without a ticket.
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2851206192
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientLazyTransaction.java:
##########
@@ -92,14 +93,7 @@ public CompletableFuture<Void> commitAsync() {
@Override
public void rollback() throws TransactionException {
- var tx0 = tx;
-
- if (tx0 == null) {
- // No operations were performed, nothing to rollback.
- return;
- }
-
- tx0.join().rollback();
+ rollbackAsync().join();
Review Comment:
https://issues.apache.org/jira/browse/IGNITE-27947
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2851629956
##########
modules/api/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -468,6 +468,9 @@ public static class Transactions {
/** Operation failed due to replication delayed ack failure. */
public static final int TX_DELAYED_ACK_ERR = TX_ERR_GROUP.registerErrorCode((short) 17);
+
+ /** Transaction was internally killed. This is retriable state. */
Review Comment:
It's retriable in terms of runInTransaction retry check, based on marker interface.
This code is added by you.
TransactionKilledException is marked with `RetriableTransactionException`.
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2852229248
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxCleanupRequestHandler.java:
##########
@@ -291,6 +291,36 @@ private void sendCleanupReplicatedResponse(UUID txId, InternalClusterNode sender
messagingService.send(sender, ChannelType.DEFAULT, prepareResponse(new CleanupReplicatedInfo(txId, partitions)));
}
+ /**
+ * Discards local write intents.
+ *
+ * @param partitions Partitions.
+ * @param txId The transaction id.
+ *
+ * @return The future.
+ */
+ CompletableFuture<Void> discardLocalWriteIntents(List<EnlistedPartitionGroup> partitions, UUID txId) {
+ Map<EnlistedPartitionGroup, CompletableFuture<?>> writeIntentSwitches = new HashMap<>();
Review Comment:
```suggestion
Map<EnlistedPartitionGroup, CompletableFuture<?>> writeIntentSwitches = new HashMap<>(partitions.size());
```
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2846754139
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -206,6 +225,79 @@ public void commit() throws TransactionException {
sync(commitAsync());
}
+ /**
+ * Discards the directly mapped transaction fragments in case of coordinator side transaction invalidation
+ * (either kill or implicit rollback due to mapping failure, see postEnlist).
+ *
+ * @param killed Killed flag.
+ *
+ * @return The future.
+ */
+ public CompletableFuture<Void> discardDirectMappings(boolean killed) {
+ enlistPartitionLock.writeLock().lock();
+
+ try {
+ if (!finishFut.compareAndSet(null, new CompletableFuture<>())) {
+ return finishFut.get();
+ }
+ } finally {
+ enlistPartitionLock.writeLock().unlock();
+ }
+
+ return sendDiscardRequests().handle((r, e) -> {
+ setState(killed ? STATE_KILLED : STATE_ROLLED_BACK);
+ ch.inflights().erase(txId());
+ this.finishFut.get().complete(null);
+ return null;
+ });
+ }
+
+ private CompletableFuture<Void> sendDiscardRequests() {
+ assert finishFut != null;
+
+ if (!ch.protocolContext().isFeatureSupported(TX_DIRECT_MAPPING_SEND_DISCARD)) {
+ return nullCompletedFuture();
+ }
+
+ Map<String, List<TablePartitionId>> enlistments = new HashMap<>();
+
+ for (Entry<TablePartitionId, CompletableFuture<IgniteBiTuple<String, Long>>> entry : enlisted.entrySet()) {
+ IgniteBiTuple<String, Long> info = entry.getValue().getNow(null);
+
+ if (info == null) {
+ continue; // Ignore incomplete enlistments.
+ }
+
+ enlistments.computeIfAbsent(info.get1(), k -> new ArrayList<>()).add(entry.getKey());
+ }
+
+ List<CompletableFuture<Void>> futures = new ArrayList<>(enlistments.size());
+
+ for (Entry<String, List<TablePartitionId>> entry : enlistments.entrySet()) {
+ CompletableFuture<Void> discardFut = reliableChannel.getNodeChannelAsync(entry.getKey()).thenCompose(ch -> {
Review Comment:
In other words, we should simply use ReliableChannel.serviceAsync with preferredNodeName set to the target node, and ensure that the server can re-route the request, if necessary.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -206,6 +225,79 @@ public void commit() throws TransactionException {
sync(commitAsync());
}
+ /**
+ * Discards the directly mapped transaction fragments in case of coordinator side transaction invalidation
+ * (either kill or implicit rollback due to mapping failure, see postEnlist).
+ *
+ * @param killed Killed flag.
+ *
+ * @return The future.
+ */
+ public CompletableFuture<Void> discardDirectMappings(boolean killed) {
+ enlistPartitionLock.writeLock().lock();
+
+ try {
+ if (!finishFut.compareAndSet(null, new CompletableFuture<>())) {
+ return finishFut.get();
+ }
+ } finally {
+ enlistPartitionLock.writeLock().unlock();
+ }
+
+ return sendDiscardRequests().handle((r, e) -> {
+ setState(killed ? STATE_KILLED : STATE_ROLLED_BACK);
+ ch.inflights().erase(txId());
+ this.finishFut.get().complete(null);
+ return null;
+ });
+ }
+
+ private CompletableFuture<Void> sendDiscardRequests() {
+ assert finishFut != null;
+
+ if (!ch.protocolContext().isFeatureSupported(TX_DIRECT_MAPPING_SEND_DISCARD)) {
+ return nullCompletedFuture();
+ }
+
+ Map<String, List<TablePartitionId>> enlistments = new HashMap<>();
+
+ for (Entry<TablePartitionId, CompletableFuture<IgniteBiTuple<String, Long>>> entry : enlisted.entrySet()) {
+ IgniteBiTuple<String, Long> info = entry.getValue().getNow(null);
+
+ if (info == null) {
+ continue; // Ignore incomplete enlistments.
+ }
+
+ enlistments.computeIfAbsent(info.get1(), k -> new ArrayList<>()).add(entry.getKey());
+ }
+
+ List<CompletableFuture<Void>> futures = new ArrayList<>(enlistments.size());
+
+ for (Entry<String, List<TablePartitionId>> entry : enlistments.entrySet()) {
+ CompletableFuture<Void> discardFut = reliableChannel.getNodeChannelAsync(entry.getKey()).thenCompose(ch -> {
Review Comment:
In other words, we should simply use `ReliableChannel.serviceAsync` with `preferredNodeName` set to the target node, and ensure that the server can re-route the request, if necessary.
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2851760406
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -75,7 +89,7 @@ public class ClientTransaction implements Transaction {
/** The future used on repeated commit/rollback. */
@IgniteToStringExclude
- private final AtomicReference<CompletableFuture<Void>> finishFut = new AtomicReference<>();
+ private final AtomicReference<CompletableFuture<Void>> finishFut = new AtomicReference<>(); // TODO use updater
Review Comment:
Replaced with volatile field.
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2851629956
##########
modules/api/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -468,6 +468,9 @@ public static class Transactions {
/** Operation failed due to replication delayed ack failure. */
public static final int TX_DELAYED_ACK_ERR = TX_ERR_GROUP.registerErrorCode((short) 17);
+
+ /** Transaction was internally killed. This is retriable state. */
Review Comment:
It's retriable in terms of runInTransaction.
This code is added by you.
TransactionKilledException is marked with `RetriableTransactionException`.
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2851629956
##########
modules/api/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -468,6 +468,9 @@ public static class Transactions {
/** Operation failed due to replication delayed ack failure. */
public static final int TX_DELAYED_ACK_ERR = TX_ERR_GROUP.registerErrorCode((short) 17);
+
+ /** Transaction was internally killed. This is retriable state. */
Review Comment:
It's retriable in terms of `runInTransaction` retry check, based on marker interface.
This code is added by you.
`TransactionKilledException `is marked with `RetriableTransactionException`.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientLazyTransaction.java:
##########
@@ -92,14 +93,7 @@ public CompletableFuture<Void> commitAsync() {
@Override
public void rollback() throws TransactionException {
- var tx0 = tx;
-
- if (tx0 == null) {
- // No operations were performed, nothing to rollback.
- return;
- }
-
- tx0.join().rollback();
+ rollbackAsync().join();
Review Comment:
Wrapped commit and rollback future in `ViewUtils.sync`. This should be enough.
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2851639262
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxCleanupRequestHandler.java:
##########
@@ -291,6 +291,36 @@ private void sendCleanupReplicatedResponse(UUID txId, InternalClusterNode sender
messagingService.send(sender, ChannelType.DEFAULT, prepareResponse(new CleanupReplicatedInfo(txId, partitions)));
}
+ /**
+ * Discards local write intents.
+ *
+ * @param partitions Partitions.
+ * @param txId The transaction id.
+ *
+ * @return The future.
+ */
+ CompletableFuture<Void> discardLocalWriteIntents(List<EnlistedPartitionGroup> partitions, UUID txId) {
+ Map<EnlistedPartitionGroup, CompletableFuture<?>> writeIntentSwitches = new HashMap<>();
+
+ for (EnlistedPartitionGroup partition : partitions) {
+ CompletableFuture<Void> future = writeIntentSwitchProcessor.switchLocalWriteIntents(
+ partition,
+ txId,
+ false,
+ null
+ ).thenAccept(this::processWriteIntentSwitchResponse);
+
+ writeIntentSwitches.put(partition, future);
+ }
+
+ releaseTxLocks(txId);
Review Comment:
Currently locks will be cleaned up on tx timeout, which is not the best solution.
There is a ticket for improvement [1]
[1] https://issues.apache.org/jira/browse/IGNITE-27651
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2846122311
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/InternalTxOptions.java:
##########
@@ -47,11 +48,16 @@ public class InternalTxOptions {
@Nullable
private final HybridTimestamp readTimestamp;
- private InternalTxOptions(TxPriority priority, long timeoutMillis, @Nullable HybridTimestamp readTimestamp, @Nullable String txLabel) {
+ /** Transaction kill closure. Defines context specific action on tx kill. */
+ private final @Nullable Consumer<InternalTransaction> killClosure;
+
+ private InternalTxOptions(TxPriority priority, long timeoutMillis, @Nullable HybridTimestamp readTimestamp, @Nullable String txLabel,
+ Consumer<InternalTransaction> killClosure) {
this.priority = priority;
Review Comment:
`killClosure` is treated as nullable (field is `@Nullable` and `defaults()` builds options without setting it), but the constructor parameter is non-null `Consumer<InternalTransaction>`. This makes the nullability contract inconsistent and can confuse callers/static analysis. Consider marking the ctor parameter (and the Builder field) as `@Nullable` and/or providing an explicit default no-op closure.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientTransactionsTest.java:
##########
@@ -1183,38 +1342,134 @@ void testReadOnlyTxDoesNotSeeUpdatesAfterStart(boolean server) {
assertNull(val, "Read-only transaction should not see values committed after its start");
}
- @Test
- public void testRollbackDoesNotBlockOnLockConflict() {
+ @ParameterizedTest
+ @MethodSource("killTestContextFactory")
+ public void testRollbackDoesNotBlockOnLockConflict(KillTestContext ctx) throws InterruptedException {
ClientTable table = (ClientTable) table();
- KeyValueView<Tuple, Tuple> kvView = table().keyValueView();
Map<Partition, ClusterNode> map = table.partitionDistribution().primaryReplicasAsync().join();
- List<Tuple> tuples0 = generateKeysForPartition(800, 10, map, 0, table);
+ List<Tuple> tuples0 = generateKeysForPartition(100, 10, map, 0, table);
ClientLazyTransaction olderTxProxy = (ClientLazyTransaction) client().transactions().begin();
ClientLazyTransaction youngerTxProxy = (ClientLazyTransaction) client().transactions().begin();
Tuple key = tuples0.get(0);
Tuple key2 = tuples0.get(1);
- Tuple val = val("1");
- Tuple val2 = val("2");
- kvView.put(olderTxProxy, key, val);
+ assertThat(ctx.put.apply(client(), olderTxProxy, key), willSucceedFast());
ClientTransaction olderTx = olderTxProxy.startedTx();
- kvView.put(youngerTxProxy, key2, val2);
+ assertThat(ctx.put.apply(client(), youngerTxProxy, key2), willSucceedFast());
ClientTransaction youngerTx = youngerTxProxy.startedTx();
assertTrue(olderTx.txId().compareTo(youngerTx.txId()) < 0);
// Older is allowed to wait with wait-die.
- CompletableFuture<Void> fut = kvView.putAsync(olderTxProxy, key2, val);
+ CompletableFuture<?> fut = ctx.put.apply(client(), olderTxProxy, key2);
assertFalse(fut.isDone());
+ // Give some time to acquire a lock to avoid a race with next rollback.
+ Thread.sleep(500);
+
Review Comment:
Using `Thread.sleep(500)` as a synchronization aid makes this integration test timing-dependent and potentially flaky (slow/loaded CI, different schedulers). Prefer awaiting a specific condition (e.g., using Awaitility on a predicate that confirms the lock conflict/wait is established) instead of sleeping a fixed duration.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -206,6 +225,79 @@ public void commit() throws TransactionException {
sync(commitAsync());
}
+ /**
+ * Discards the directly mapped transaction fragments in case of coordinator side transaction invalidation
+ * (either kill or implicit rollback due to mapping failure, see postEnlist).
+ *
+ * @param killed Killed flag.
+ *
+ * @return The future.
+ */
+ public CompletableFuture<Void> discardDirectMappings(boolean killed) {
+ enlistPartitionLock.writeLock().lock();
+
+ try {
+ if (!finishFut.compareAndSet(null, new CompletableFuture<>())) {
+ return finishFut.get();
+ }
+ } finally {
+ enlistPartitionLock.writeLock().unlock();
+ }
+
+ return sendDiscardRequests().handle((r, e) -> {
+ setState(killed ? STATE_KILLED : STATE_ROLLED_BACK);
+ ch.inflights().erase(txId());
+ this.finishFut.get().complete(null);
+ return null;
+ });
+ }
+
+ private CompletableFuture<Void> sendDiscardRequests() {
+ assert finishFut != null;
Review Comment:
The `assert finishFut != null;` in `sendDiscardRequests()` is ineffective because `finishFut` is an `AtomicReference` field that is always non-null. If the intent is to assert that the transaction is already in finishing state, assert `finishFut.get() != null` (or drop the assert).
```suggestion
assert finishFut.get() != null;
```
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransactionKilledException.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.client.tx;
+
+import java.util.UUID;
+import org.apache.ignite.tx.TransactionException;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Reports a killed transaction.
+ */
+public class ClientTransactionKilledException extends TransactionException {
+ /** Serial version uid. */
+ private static final long serialVersionUID = 0L;
+
+ /** Transaction id. */
+ private final UUID txId;
+
+ /**
+ * Constructor.
+ *
+ * @param traceId Trace ID.
+ * @param code Error code.
+ * @param message String message.
+ * @param txId Related transaction id.
+ * @param cause The cause.
+ */
+ public ClientTransactionKilledException(UUID traceId, int code, @Nullable String message, UUID txId, @Nullable Throwable cause) {
+ super(traceId, code, message, cause);
+
+ this.txId = txId;
+ }
+
+ /**
+ * Constructor (for copying purposes).
+ *
+ * @param traceId Trace ID.
+ * @param code Error code.
+ * @param message String message.
+ * @param cause The cause.
+ */
+ public ClientTransactionKilledException(UUID traceId, int code, @Nullable String message, @Nullable Throwable cause) {
+ super(traceId, code, message, cause);
+
+ this.txId = cause instanceof ClientTransactionKilledException
+ ? ((ClientTransactionKilledException) cause).txId : null;
+ }
+
+ /**
+ * Returns a related transaction id.
+ *
+ * @return The id.
+ */
+ public UUID txId() {
+ return txId;
+ }
Review Comment:
`txId` can become `null` in the copy constructor (when `cause` is not a `ClientTransactionKilledException`), but the field and accessor are non-null (`UUID`) and callers (e.g., notification handling) assume a non-null id. Consider making `txId`/`txId()` `@Nullable` or enforcing a non-null `txId` in all constructors (e.g., require/derive it deterministically) to avoid potential NPEs and clarify the contract.
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -383,35 +499,34 @@ public CompletableFuture<IgniteBiTuple<String, Long>> enlistFuture(ReliableChann
throw new TransactionException(TX_ALREADY_FINISHED_ERR, format("Transaction is already finished [tx={}].", this));
Review Comment:
In `enlistFuture`, when `tryLock()` fails you always throw `TX_ALREADY_FINISHED_ERR`. This can misreport the error for killed transactions (state `STATE_KILLED`) since you already have `exceptionForState(...)` to return `TX_KILLED_ERR`. Consider checking the current state/`finishFut` and throwing `exceptionForState(state.get(), this)` (or otherwise preserving the killed vs finished distinction) on the lock-acquisition failure path.
```suggestion
throw exceptionForState(state.get(), this);
```
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2846919463
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -75,7 +89,7 @@ public class ClientTransaction implements Transaction {
/** The future used on repeated commit/rollback. */
@IgniteToStringExclude
- private final AtomicReference<CompletableFuture<Void>> finishFut = new AtomicReference<>();
+ private final AtomicReference<CompletableFuture<Void>> finishFut = new AtomicReference<>(); // TODO use updater
Review Comment:
The field is always updated under the write lock.
Can we just replace wrapper object with volatile field?
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2851691951
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -206,6 +225,79 @@ public void commit() throws TransactionException {
sync(commitAsync());
}
+ /**
+ * Discards the directly mapped transaction fragments in case of coordinator side transaction invalidation
+ * (either kill or implicit rollback due to mapping failure, see postEnlist).
+ *
+ * @param killed Killed flag.
+ *
+ * @return The future.
+ */
+ public CompletableFuture<Void> discardDirectMappings(boolean killed) {
+ enlistPartitionLock.writeLock().lock();
+
+ try {
+ if (!finishFut.compareAndSet(null, new CompletableFuture<>())) {
+ return finishFut.get();
+ }
+ } finally {
+ enlistPartitionLock.writeLock().unlock();
+ }
+
+ return sendDiscardRequests().handle((r, e) -> {
+ setState(killed ? STATE_KILLED : STATE_ROLLED_BACK);
+ ch.inflights().erase(txId());
+ this.finishFut.get().complete(null);
+ return null;
+ });
+ }
+
+ private CompletableFuture<Void> sendDiscardRequests() {
+ assert finishFut != null;
+
+ if (!ch.protocolContext().isFeatureSupported(TX_DIRECT_MAPPING_SEND_DISCARD)) {
+ return nullCompletedFuture();
+ }
+
+ Map<String, List<TablePartitionId>> enlistments = new HashMap<>();
+
+ for (Entry<TablePartitionId, CompletableFuture<IgniteBiTuple<String, Long>>> entry : enlisted.entrySet()) {
+ IgniteBiTuple<String, Long> info = entry.getValue().getNow(null);
+
+ if (info == null) {
+ continue; // Ignore incomplete enlistments.
+ }
+
+ enlistments.computeIfAbsent(info.get1(), k -> new ArrayList<>()).add(entry.getKey());
+ }
+
+ List<CompletableFuture<Void>> futures = new ArrayList<>(enlistments.size());
+
+ for (Entry<String, List<TablePartitionId>> entry : enlistments.entrySet()) {
+ CompletableFuture<Void> discardFut = reliableChannel.getNodeChannelAsync(entry.getKey()).thenCompose(ch -> {
Review Comment:
Actually, re-routing logic is not necessary in this scenario and only complicates server-side implementation.
If a client connection is lost, directly mapped tx resource will be cleaned up by other measures.
I've changed the code so it uses only an existing connection.
However, I don't understand why it's forbidden to re-establish a new connection here. It's perfectly fine to me and can help address temporary network issues.
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2846751997
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientLazyTransaction.java:
##########
@@ -92,14 +93,7 @@ public CompletableFuture<Void> commitAsync() {
@Override
public void rollback() throws TransactionException {
- var tx0 = tx;
-
- if (tx0 == null) {
- // No operations were performed, nothing to rollback.
- return;
- }
-
- tx0.join().rollback();
+ rollbackAsync().join();
Review Comment:
AFAIR, we agree to avoid usage `join()` for futures that may be incomplete, because it is uninterruptable.
So, we've implemented `ViewUtils.sync(CompletableFuture<T> fut)`.
I see this approach used in the whole class and should be fixed in another ticket.
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2846919463
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -75,7 +89,7 @@ public class ClientTransaction implements Transaction {
/** The future used on repeated commit/rollback. */
@IgniteToStringExclude
- private final AtomicReference<CompletableFuture<Void>> finishFut = new AtomicReference<>();
+ private final AtomicReference<CompletableFuture<Void>> finishFut = new AtomicReference<>(); // TODO use updater
Review Comment:
The field is always updated via write lock.
Can we just replace wrapper object with volatile field?
--
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-27838 Fix directly mapped transactions rollback on enlistment failure [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #7588:
URL: https://github.com/apache/ignite-3/pull/7588#discussion_r2851629956
##########
modules/api/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -468,6 +468,9 @@ public static class Transactions {
/** Operation failed due to replication delayed ack failure. */
public static final int TX_DELAYED_ACK_ERR = TX_ERR_GROUP.registerErrorCode((short) 17);
+
+ /** Transaction was internally killed. This is retriable state. */
Review Comment:
It's retriable in terms of runInTransaction retry chech, based on marker interface.
This code is added by you.
TransactionKilledException is marked with `RetriableTransactionException`.
--
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