SEZ9 commented on PR #11757: URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5578072034
Thanks for the self-correction and for going back through the findings point by point against `39312b92d` — agreed, the stale-`taskDone` branch is a real gap and I'm treating it as the blocker. On the specific point (F2): you're right that when `finishExecutionContext` returns `false`, `finishOwnedResources` (`TaskExecutionService.java:1542-1564`) only calls `recycleClassLoader` and returns, without touching the old generation's own `taskAsyncFunctionFuture`/`timerFlushFutures` entries. Because both maps are keyed by `taskGroupLocation` alone, that branch can neither distinguish nor safely cancel "its own" entries without risking the active generation's futures. This is not fixed yet — the empty-commit CI retriggers on `454896e36` and `eb7beaa28` carried no code change, so the head under review is still the one you described. What I think is needed to close it, and what I'd ask to see in the next push: - Give the async-function and timer-flush registrations a generation identity (e.g. keyed by the same context/generation token that `finishExecutionContext` already compares), so the stale branch can cancel exactly the futures the finishing generation registered and leave the active generation's untouched. - In the stale branch, perform that cancellation alongside `recycleClassLoader` rather than returning early, so a superseded generation doesn't leak its futures until the node restarts. - A test covering the stale path: deploy gen A, deploy gen B for the same location, complete A's `taskDone`, and assert A's async/timer futures are cancelled while B's remain live — with the injected entries cleaned up afterward so the shared service doesn't carry state into later tests (that was the concern in F5). One request for you, @DanielLeens: since your review says "the rest are addressed or non-blocking", could you list which of the earlier findings you consider closed on `39312b92d` and which you're marking non-blocking? I'd like the F1/F4 lock-scope question (cancelling futures under the service-wide monitor) and the F3/F6 `cancellationFutures` handling explicitly accounted for either way, so we don't repeat the earlier miss when the F2 fix lands. I'll re-review as soon as a commit addressing the stale-generation cancellation is up; no need to retrigger CI again until then. <!-- streview-comment:877 --> -- 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]
