Copilot commented on code in PR #7980:
URL: https://github.com/apache/ignite-3/pull/7980#discussion_r3073132939
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TransactionIds.java:
##########
@@ -86,4 +86,36 @@ private static long combine(int nodeId, TxPriority priority)
{
// Shift the int 32 bits and combine with the boolean
return ((long) nodeId << 32) | priorityAsInt;
}
+
+ public static int hash(UUID txId, int divisor) {
+ return spread(txId.hashCode()) % divisor;
+ }
Review Comment:
`TransactionIds.hash` will throw `ArithmeticException` for `divisor <= 0`
and uses `%` which is easy to misuse from callers. Since this is a public
utility, consider validating `divisor > 0` (and/or using `Math.floorMod(...)`)
to make the contract explicit and avoid surprising failures if the method is
reused outside the current `CONCURRENCY` usage.
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/handlers/TxFinishReplicaRequestHandler.java:
##########
@@ -224,12 +224,18 @@ 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);
+ } catch (Exception e) {
+ LOG.warn("Failed to cleanup a transaction [id=" + txId
+ ']', e);
+ }
+
+ return txResult;
});
Review Comment:
`finishAndCleanup` no longer waits for `txManager.cleanup(...)`, but the
current code also drops the returned future entirely
(`cleanup(...).thenApply(...)` is not returned or observed). This makes cleanup
failures invisible (the `try/catch` won’t catch async failures) and can leave
resources/locks uncleaned without any signal. Consider explicitly attaching a
`whenComplete`/logging handler (and/or returning/chaining the cleanup future if
callers must observe completion) instead of creating an unused stage.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -651,32 +649,8 @@ private <R> CompletableFuture<R> trackingInvoke(
if (req.isWrite()) {
// Track only write requests from explicit transactions.
if (!tx.remote() &&
!transactionInflights.addInflight(tx.id())) {
- TxStateMeta txStateMeta = txManager.stateMeta(tx.id());
- Throwable cause = txStateMeta == null ? null :
txStateMeta.lastException();
- boolean isFinishedDueToTimeout = txStateMeta == null
- ? tx.isRolledBackWithTimeoutExceeded()
- : txStateMeta.isFinishedDueToTimeoutOrFalse();
- boolean isFinishedDueToError = !isFinishedDueToTimeout
- && txStateMeta != null
- && txStateMeta.lastExceptionErrorCode() != null;
- Throwable publicCause = isFinishedDueToError ? cause :
null;
- Integer causeErrorCode = txStateMeta == null ? null :
txStateMeta.lastExceptionErrorCode();
- int code =
finishedTransactionErrorCode(isFinishedDueToTimeout, isFinishedDueToError);
-
- return failedFuture(
- new TransactionException(code, format(
- finishedTransactionErrorMessage(
- isFinishedDueToTimeout,
- isFinishedDueToError,
- causeErrorCode,
- publicCause != null
- )
- + " [tableName={}, partId={},
txState={}, timeoutExceeded={}].",
- tableName,
- partId,
- tx.state(),
- isFinishedDueToTimeout
- ), publicCause));
+ // TODO IGNITE-28461 fail fast if TxContext.err != null.
+ return failedFuture(tx.enlistFailedException());
Review Comment:
When `addInflight` fails, the code now returns `tx.enlistFailedException()`
which does not include the table/partition context that was previously present
in this failure path (tableName/partId). This makes troubleshooting much harder
for users. Consider preserving that context in the thrown exception message
(e.g., wrap/enrich `enlistFailedException()` with tableName/partId).
```suggestion
TransactionException enlistFailedException =
tx.enlistFailedException();
return failedFuture(new TransactionException(
enlistFailedException.traceId(),
enlistFailedException.code(),
format(
"{} [tableName={}, partitionId={}]",
enlistFailedException.getMessage(),
tableName,
partId
),
enlistFailedException
));
```
##########
modules/transactions/src/test/java/org/apache/ignite/internal/tx/TransactionIdsTest.java:
##########
@@ -41,4 +44,13 @@ void transactionIdIsBuiltCorrectly(TxPriority priority) {
assertThat(extractedNodeId, is(1));
assertThat(extractedPriority, is(priority));
}
+
+ @RepeatedTest(10)
+ public void testHash() {
+ Random r = new Random(0);
+ UUID id = UUID.randomUUID();
+ int div = 1 + r.nextInt(32);
+ int hash = TransactionIds.hash(id, div);
+ assertTrue(hash >= 0 && hash < div, id + " " + div);
Review Comment:
`testHash` seeds `Random` inside the test (`new Random(0)`), so every
repetition uses the same `div` value; the `@RepeatedTest(10)` doesn’t increase
coverage. Consider moving the `Random` outside the test method (or varying the
seed/divisor) so repetitions actually exercise different divisors/inputs.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -2326,16 +2301,7 @@ private ReplicaRequest upsertAllInternal(
* @return True if retrying is possible, false otherwise.
*/
private static boolean exceptionAllowsImplicitTxRetry(Throwable e) {
- return matchAny(
- unwrapCause(e),
- ACQUIRE_LOCK_ERR,
- GROUP_OVERLOADED_ERR,
- REPLICA_MISS_ERR,
- REPLICA_UNAVAILABLE_ERR,
- REPLICA_ABSENT_ERR,
- PRIMARY_REPLICA_AWAIT_ERR,
- PRIMARY_REPLICA_AWAIT_TIMEOUT_ERR
- );
+ return ExceptionUtils.hasCause(e, RetriableTransactionException.class);
Review Comment:
`exceptionAllowsImplicitTxRetry` now relies solely on the
`RetriableTransactionException` marker. Some errors that were previously
retried here (e.g. `GROUP_OVERLOADED_ERR` via `GroupOverloadedException`) do
not implement this marker, so implicit-tx retries may silently stop happening.
Either ensure all previously-retriable exceptions implement
`RetriableTransactionException`, or keep a small explicit allowlist fallback
for the relevant error codes.
```suggestion
if (ExceptionUtils.hasCause(e, RetriableTransactionException.class))
{
return true;
}
for (Throwable cause = e; cause != null; cause = cause.getCause()) {
if (cause instanceof IgniteException
&& ((IgniteException) cause).code() ==
org.apache.ignite.lang.ErrorGroups.Replicator.GROUP_OVERLOADED_ERR) {
return true;
}
}
return false;
```
--
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]
