DanielLeens commented on PR #11924:
URL: https://github.com/apache/seatunnel/pull/11924#issuecomment-5476937193
Thanks for the careful re-check, @JeremyXin — replying to your three points
(checked against the current head `4a5e69f1c08a`, which only merges in `dev` to
resolve the branch conflicts you flagged; it does not touch
`TaskExecutionService.java` at all, so my open Issue 1 from the 2026-08-25
review is still unaddressed there):
1. **Coordination with #11999**: confirmed, this is a real overlap — both
fix #11755 by changing `taskDone()` in the same file. They're not identical in
scope though: #11999 keeps the original `TaskGroupExecutionTracker` (no
`ExecutionGeneration` concept) and adds a `synchronized` block plus a
`TaskGroupContext`-identity gate
(`finishOwnedResources`/`finishExecutionContext`) around the existing
recycle/cancel calls. It doesn't introduce the per-generation
`timerFlushFuturesByGeneration`/`taskAsyncFunctionFutureByGeneration`
registries this PR adds, so it doesn't inherit this PR's still-open Issue 1
concurrency race — but it also doesn't close the timer-flush/async-function
future-leak scenario this PR's own
`testTimerFlushCloseOnlyCancelsOwnedExecutionGeneration` regression test
targets. This looks like a genuine design fork (simpler synchronized-gate vs.
generation-scoped registries), not a trivial duplicate, so it's worth a
maintainer call on which shape to keep rather than
trying to reconcile them line-by-line.
2. **Dead code in `closeTimerFlushTaskForGeneration`**: confirmed, you're
right. Current head, `TaskExecutionService.java:1202-1208`:
```java
ScheduledFuture<?> future = generationFutures.remove(taskLocation);
if (future == null) {
return;
}
if (future != null && !future.isDone()) { // future != null is
unreachable here
future.cancel(false);
}
```
The `future != null` re-check is dead code — the method already returned
above if `future` were null. Please simplify to `if (!future.isDone())`.
3. **Are `cancelAsyncFunction`/`cancelTimerFlushForTaskGroup` reached on all
completion paths, including exceptions?** Yes — traced `taskDone()` at
`TaskExecutionService.java:1646-1690`: both calls sit unconditionally inside
the `completionLatch.decrementAndGet() == 0` block, each wrapped in its own
`try { } catch (Throwable t) { logger.severe(...) }`, and both run *before* the
branch on `ex == null` / `isCancel.get()` that decides
FINISHED/CANCELED/FAILED. So they fire on every terminal path — success,
cancel, and failure — and a `Throwable` from one doesn't skip the other or
block the final state transition. No unbounded-growth risk from a skipped
cleanup call on that particular axis.
Branch conflicts are resolved as of the latest commit. My Issue 1 (High,
blocking — the same non-atomic `isEmpty()`-then-`outerMap.remove(key,map)`
pattern still present in 5 spots, e.g. `TaskExecutionService.java:1184-1185,
1211-1212, 1215-1216`) from the 2026-08-25 review remains open in the current
head. Happy to do a fresh full re-review once that lands alongside a decision
on how this coordinates with #11999.
--
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]
