DanielLeens commented on PR #11757: URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5248804552
@nzw921rx Agreed, and thanks for raising it — this is exactly right and it reinforces a point I already made a blocker in my CHANGES_REQUESTED review above (Issue 3): the PR description cites `TaskDeployStaleContextRaceTest` as covering this fix, but that class only exists on the still-open, unmerged #11727, not on `dev`. As things stand, this PR would land a concurrency fix with zero verification of its own. Your framing adds a concrete reason on top of mine: even setting aside that the test doesn't exist yet, borrowing coverage from a *different* PR is fragile by construction. #11727 and #11757 have no ordering guarantee — either could merge first, get rebased, or have its test changed/dropped independently — so a "shared" test is really coverage that belongs to neither PR reliably. If #11727 merges first with a different final shape for that test (or none at all), #11757 would be relying on protection that was never actually committed alongside it. So to be explicit: yes, this stays a blocking item, not a nice-to-have. My original Issue 3 already asked for a dedicated regression test in this PR's own diff; your comment is additive in explaining *why* a borrowed test doesn't satisfy that bar even if #11727's test happens to exist by the time this merges. I sketched one deterministic option in my review (deploy gen 1, capture its context, install a distinct gen-2 context for the same `TaskGroupLocation`, drive gen 1's tracker through `taskDone()`, then assert the gen-2 entry, its class loaders, and its cancellation future all survive) — no threads, no sleeps needed. That test should live in this PR's `seatunnel-engine-server/src/test/java` regardless of how #11727 evolves. To be clear, this is one of several High-severity blockers in my review (recycleClassLoader() still resolving by location, the unguarded cancellationFutures/async/timer teardown, and CI red on spotless), so a local UT alone won't be sufficient to merge — but it's a necessary piece, and I don't want it to get lost. Thanks again for pushing on this. -- 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]
