DanielLeens commented on PR #11999: URL: https://github.com/apache/seatunnel/pull/11999#issuecomment-5473722146
Hi @JeremyXin, thanks for flagging this - you're right, and it's actually a three-way overlap, not just two-way. I compared the actual diffs of all three PRs against `TaskExecutionService.java` and here's what I found: **#11757 and #11999 are the same fix, not two competing implementations.** #11999's own description says it "continues #11757," and the git history backs that up: the first three commits on #11999 are byte-identical to what's currently on #11757, preserving @waterWang's original authorship. The only new commit on #11999 adds one extra step - releasing the stale generation's own class loader before returning - closing the one gap I'd asked for across earlier review rounds on #11757. So these two just need reconciling: merge #11999 (it's a clean rebase onto `dev`; #11757 currently shows as `dirty`/conflicting), then close #11757 as superseded with a pointer comment so @waterWang keeps credit for the original fix. **#11924 takes a different, broader approach to the same root cause (#11755).** #11757/#11999 guard the shared `executionContexts`/`cancellationFutures` maps with an identity check so a stale generation's completion can't evict a newer generation's active context. #11924 goes further: it introduces an explicit `ExecutionGeneration` token threaded through `TaskExecutionContext`, and uses it to scope the async-function-future and timer-flush-future registries per generation as well, not just per `TaskGroupLocation`. That's a real additional edge of the same bug class - with the narrower #11757/#11999 fix, those two registries stay keyed only by location, so a still-in-flight generation's async/timer resources could in principle still be reached by a differently-timed cleanup call from a sibling generation sharing that location. The tradeoff is a larger surface area: a new public nested type and several call-site signature changes across roughly six files. **#11924 also isn't mergeable yet on its own merits, independent of this overlap.** Its current head still carries an open blocking issue from my last review round: several of the new generation-scoped cleanup paths use a non-atomic `isEmpty()`-then-`remove(key, map)` pattern that a concurrent register call can race. One instance of exactly this was already found and fixed, but about five structurally identical spots are still open as of today. **Suggested path forward:** merge #11999 first, since it's the more minimal, already-reviewed fix for the core race and is ready to go; close #11757 as superseded. #11924's additional per-registry generation-scoping for async/timer futures is a worthwhile follow-up hardening on top of whatever lands from #11999, once its own open concurrency issue is fixed and it's rebased onto the new baseline. @BinTaoMa @zhangshenghang - happy to take another pass on either PR once you've had a chance to look at this; wanted to lay out the actual technical difference between the two approaches before suggesting a direction rather than just picking one. -- 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]
