Copilot commented on code in PR #7999:
URL: https://github.com/apache/ignite-3/pull/7999#discussion_r3098210896
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/DefaultTablePartitionReplicaProcessor.java:
##########
@@ -1669,17 +1772,23 @@ private <T> CompletableFuture<T> continueResolvingByPk(
* Appends an operation to prevent the race between commit/rollback and
the operation execution.
*
* @param txId Transaction id.
+ * @param commitPartitionId Commit partition id.
* @param requestType Request type.
* @param full {@code True} if a full transaction and can be immediately
committed.
* @param op Operation closure.
* @return A future object representing the result of the given operation.
*/
private <T> CompletableFuture<T> appendTxCommand(
UUID txId,
+ @Nullable ZonePartitionId commitPartitionId,
RequestType requestType,
boolean full,
Supplier<CompletableFuture<T>> op
) {
+ PendingTxContext txCtx = requestType.isRwRead() ? null :
+ pendingTransactions.computeIfAbsent(txId,
+ id -> new PendingTxContext(new CompletableFuture<>(),
commitPartitionId));
Review Comment:
`appendTxCommand` accepts a `@Nullable` `commitPartitionId`, but for
non-RW-read requests it is stored into `PendingTxContext` and later used for
tx-state resolution. Relying on `assert commitPartId != null` in
`PendingTxContext` is unsafe when assertions are disabled and can lead to NPEs
at runtime. Consider enforcing non-null explicitly (e.g.,
`Objects.requireNonNull`) when `!requestType.isRwRead()` and/or making the
parameter non-null for those request types.
```suggestion
pendingTransactions.computeIfAbsent(txId, id -> new
PendingTxContext(
new CompletableFuture<>(),
requireNonNull(commitPartitionId, "commitPartitionId
must not be null for non-RW-read requests")
));
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/DefaultTablePartitionReplicaProcessor.java:
##########
@@ -4085,4 +4205,20 @@ public WriteIntentResolutionResult(boolean
writeIntentReadable, TransactionMeta
this.transactionMeta = transactionMeta;
}
}
+
+ /**
+ * Represents waiting for cleanup transactions context.
+ */
+ private static class PendingTxContext {
+ final CompletableFuture<Void> cleanupFut;
+ final ZonePartitionId commitPartId;
+
+ PendingTxContext(CompletableFuture<Void> cleanupFut, ZonePartitionId
commitPartId) {
+ assert cleanupFut != null;
+ assert commitPartId != null;
+
+ this.cleanupFut = cleanupFut;
+ this.commitPartId = commitPartId;
Review Comment:
`PendingTxContext` uses `assert` statements for null checks on constructor
args. Because assertions may be disabled in production, these checks won’t
protect against nulls and could result in later NPEs (e.g., during tx-state
resolution). Prefer hard validation (e.g., `Objects.requireNonNull`) and/or
non-null annotations on fields/params.
```suggestion
this.cleanupFut = requireNonNull(cleanupFut, "cleanupFut");
this.commitPartId = requireNonNull(commitPartId, "commitPartId");
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/DefaultTablePartitionReplicaProcessor.java:
##########
@@ -581,6 +597,80 @@ private void replicaTouch(UUID txId, UUID coordinatorId,
ZonePartitionId commitP
.build());
}
+ private <T> CompletableFuture<T> resolvePendingTransactions(
+ HybridTimestamp requestTime,
+ HybridTimestamp currentSafeTime,
+ Function<HybridTimestamp, CompletableFuture<T>> action
+ ) {
+ // Wait for currentSafeTime >= requestTime to avoid out-of-order
transactions arrival.
+ if (currentSafeTime.compareTo(requestTime) < 0) {
+ return safeTime.waitFor(requestTime)
+ .thenComposeAsync(
+ unused -> resolvePendingTransactions(requestTime,
safeTime.current(), action),
+ partitionOperationsExecutor);
+ }
+
+ assert currentSafeTime.compareTo(requestTime) >= 0 : "currentSafeTime
< lowerBoundTimestamp";
+ assert currentSafeTime.compareTo(safeTime.current()) <= 0 :
"currentSafeTime > safeTime";
+
+ // Stable committed snapshot is ensured after resolving pending
transactions state.
+ UUID uppedBoundTxId = TransactionIds.transactionId(currentSafeTime,
Integer.MAX_VALUE, TxPriority.NORMAL);
+ ConcurrentNavigableMap<UUID, PendingTxContext> txToWait =
pendingTransactions.headMap(uppedBoundTxId, true);
Review Comment:
Variable name `uppedBoundTxId` looks like a typo (probably meant
`upperBoundTxId`). Renaming would improve readability since this value is used
as an upper bound for `pendingTransactions.headMap(...)`.
```suggestion
UUID upperBoundTxId = TransactionIds.transactionId(currentSafeTime,
Integer.MAX_VALUE, TxPriority.NORMAL);
ConcurrentNavigableMap<UUID, PendingTxContext> txToWait =
pendingTransactions.headMap(upperBoundTxId, true);
```
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -327,7 +339,12 @@ public CompletableFuture<Void> commitAsync() {
if (finishFut != null) {
return finishFut;
} else {
- finishFut = new CompletableFuture<>();
+ if (implicitRollbackFut != null) {
+ finishFut = nullCompletedFuture();
+ return implicitRollbackFut;
+ } else {
+ finishFut = new CompletableFuture<>();
+ }
Review Comment:
In `commitAsync`, when `implicitRollbackFut` is already set you assign
`finishFut` to a successfully-completed future and return
`implicitRollbackFut`. This can make subsequent `commitAsync()` calls complete
successfully (via `finishFut`) even though the transaction has been rolled
back/killed, and it can also hide an implicit rollback failure on repeated
finish calls. Consider making `commitAsync` fail immediately based on the
current state (rolled back/killed), or have `finishFut` reflect the same
completion (success/exception) as the implicit rollback instead of
`nullCompletedFuture()`.
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/handlers/TxFinishReplicaRequestHandler.java:
##########
@@ -224,12 +224,22 @@ private CompletableFuture<TransactionResult>
finishAndCleanup(
.map(entry -> new EnlistedPartitionGroup(entry.getKey(),
entry.getValue().tableIds()))
.collect(toList());
return finishTransaction(enlistedPartitionGroups, txId, commit,
commitTimestamp)
- .thenCompose(txResult -> {
+ .thenApply(txResult -> {
boolean actualCommit = txResult.transactionState() ==
COMMITTED;
HybridTimestamp actualCommitTs =
txResult.commitTimestamp();
- return txManager.cleanup(replicationGroupId,
enlistedPartitions, actualCommit, actualCommitTs, txId)
- .thenApply(v -> txResult);
+ try {
+ txManager.cleanup(replicationGroupId,
enlistedPartitions, actualCommit, actualCommitTs, txId)
+ .thenApply(v -> txResult).whenComplete((r, e)
-> {
+ if (e != null) {
+ LOG.warn("Failed to cleanup a
transaction [id=" + txId + ']', e);
+ }
+ });
+ } catch (Exception e) {
+ LOG.warn("Failed to cleanup a transaction [id=" + txId
+ ']', e);
Review Comment:
Logging style: this introduces string concatenation in `LOG.warn("Failed to
cleanup a transaction [id=" + txId + ']', e)`. Elsewhere in this module logging
typically uses parameterized messages with the throwable passed separately
(e.g. `WriteIntentSwitchRequestHandler`). Using the parameterized form avoids
eager string building and keeps logs consistent.
```suggestion
LOG.warn("Failed to cleanup a
transaction [id={}]", txId, e);
}
});
} catch (Exception e) {
LOG.warn("Failed to cleanup a transaction [id={}]",
txId, e);
```
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransaction.java:
##########
@@ -421,8 +438,15 @@ private CompletableFuture<Void> rollbackAsyncInternal() {
if (finishFut != null) {
return finishFut;
} else {
- finishFut = new CompletableFuture<>();
+ if (implicitRollbackFut != null) {
+ finishFut = nullCompletedFuture();
+ return implicitRollbackFut;
+ } else {
+ finishFut = new CompletableFuture<>();
+ }
Review Comment:
Similar to `commitAsync`, `rollbackAsyncInternal` sets `finishFut` to a
successfully-completed future when `implicitRollbackFut` is non-null and
returns `implicitRollbackFut`. If the implicit rollback completes
exceptionally, subsequent `rollbackAsync()` calls will return `finishFut` and
appear successful. `finishFut` should not be set to a success placeholder in
this branch; it should either represent the same outcome as the implicit
rollback, or `rollbackAsyncInternal` should return an already-failed future
based on the current tx state.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -235,23 +235,23 @@ public CompletableFuture<Void> finish(
* @param commit Commit flag.
* @param executionTimestamp The timestamp is the time when the
transaction is applied to the remote node.
* @param full Full state transaction marker.
- * @param isComplete The flag is true if the transaction is completed
through the public API, false for {@link this#kill()} invocation.
+ * @param isFinishedDirectly {@code True} if the transaction is completed
through the public API, false for {@link #kill()}.
Review Comment:
Javadoc: `{@code True}` should be lowercase (`{@code true}`) to match Java
boolean literals and surrounding documentation style.
```suggestion
* @param isFinishedDirectly {@code true} if the transaction is
completed through the public API, false for {@link #kill()}.
```
--
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]
