Copilot commented on code in PR #7779:
URL: https://github.com/apache/ignite-3/pull/7779#discussion_r2938319331
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/table/ClientTableCommon.java:
##########
@@ -538,6 +539,24 @@ public static TableNotFoundException
tableIdNotFoundException(Integer tableId) {
MESSAGE_TX_ALREADY_FINISHED_DUE_TO_TIMEOUT + " [tx=" + remote + "].");
}
+ // Track this remote enlistment for cleanup if
client disconnects.
+ try {
+ resources.addTxCleaner(txId, tableId,
commitPart, txManager, (IgniteTablesInternal) tables);
+ } catch (IgniteInternalCheckedException e) {
+ // Client disconnected (resource registry
closed).
+ try {
+ remote.rollback();
+ } catch (Exception ex) {
+ e.addSuppressed(ex);
+ }
+
+ throw new IgniteException(e.traceId(),
e.code(), "Client disconnected, tx rolled back: " + remote, e);
+ }
+
+ // Stop tracking on tx finish.
+ txManager.resourceRegistry().register(
+ new FullyQualifiedResourceId(txId, txId),
txId, () -> () -> resources.removeTxCleaner(txId));
Review Comment:
In this register() call, the second argument (`remoteHostId`) is supposed to
be the cluster node UUID that created/triggered the resource. Passing `txId`
here will make ResourceVacuumManager treat it as an orphan (no such node in
topology) and close it on the next vacuum cycle, which can remove the cleaner
while the tx is still active and break disconnect cleanup. Use the coordinator
node id (`coord`) or another real cluster node id as `remoteHostId` instead of
`txId`.
##########
modules/core/src/main/java/org/apache/ignite/internal/util/ExceptionUtils.java:
##########
@@ -455,6 +456,31 @@ private static boolean hasCauseOrSuppressedInternal(
return false;
}
+ /**
+ * Checks if the given throwable is already present in the cause or
suppressed hierarchy of the given throwable.
+ *
+ * @param t Throwable.
+ * @param dejaVu Known throwables.
+ * @return True if seen before, false otherwise.
+ */
+ public static boolean existingCauseOrSuppressed(Throwable t,
HashSet<Throwable> dejaVu) {
+ if (t == null) {
+ return false;
+ }
+
+ if (!dejaVu.add(t)) {
+ return true;
+ }
+
+ for (Throwable sup : t.getSuppressed()) {
+ if (existingCauseOrSuppressed(sup, dejaVu)) {
+ return true;
+ }
+ }
+
+ return existingCauseOrSuppressed(t.getCause(), dejaVu);
+ }
Review Comment:
The new `existingCauseOrSuppressed` helper exposes `HashSet<Throwable>` in
its public signature and even accepts `t == null` while the parameter isn't
marked nullable. Consider changing the signature to use `Set<Throwable>` (and
annotate `t` as `@Nullable` if null is supported) so callers can reuse the
IdentityHashMap-backed Set pattern already used elsewhere in this class and so
the API isn't tied to a concrete collection type.
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientResourceRegistry.java:
##########
@@ -112,6 +123,44 @@ public ClientResource remove(long id) throws
IgniteInternalCheckedException {
}
}
+ /**
+ * Records that a remote transaction enlisted a partition on this node.
+ *
+ * @param txId Transaction ID.
+ * @param tableId Table ID.
+ * @param partitionId Partition ID.
Review Comment:
The Javadoc for `addTxCleaner` doesn't mention the `txManager` and `tables`
parameters that are part of the method signature. Please update the comment so
it documents all parameters (or adjust the signature if those should be stored
in the registry instead).
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientResourceRegistry.java:
##########
@@ -122,24 +171,35 @@ public void close() {
busyLock.block();
- IgniteInternalException ex = null;
+ AtomicReference<IgniteInternalException> ex = new AtomicReference<>();
+ var dejaVu = new HashSet<Throwable>();
- for (ClientResource r : res.values()) {
+ Consumer<Runnable> releaseSafe = r -> {
try {
- r.release();
- } catch (Exception e) {
- if (ex == null) {
- ex = new IgniteInternalException(e);
- } else {
- ex.addSuppressed(e);
+ r.run();
+ } catch (Throwable e) {
+ if (ex.get() == null) {
+ ex.set(new IgniteInternalException(e));
+ existingCauseOrSuppressed(e, dejaVu); // Seed dejaVu.
+ } else if (!existingCauseOrSuppressed(e, dejaVu)) {
+ ex.get().addSuppressed(e);
}
}
+ };
+
+ for (ClientResource r : res.values()) {
+ releaseSafe.accept(r::release);
}
res.clear();
- if (ex != null) {
- throw ex;
+ for (var cleaner : txCleaners.values()) {
+ // Don't block the thread, clean in background.
discardLocalWriteIntents swallows errors anyway.
+ releaseSafe.accept(cleaner::clean);
Review Comment:
`releaseSafe` accepts a `Runnable`, but
`ClientTxPartitionEnlistmentCleaner.clean()` returns `CompletableFuture<Void>`,
so `cleaner::clean` is not compatible with `Runnable` and should not compile.
Wrap it in a void-compatible lambda (e.g., invoke `clean()` inside `Runnable`)
or change `releaseSafe` to accept a functional type that can return a value
(Supplier/Callable).
--
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]
