DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5473705341
Confirmed the fix landed, not just the theory, @abdessalems. I checked the actual diff between the head I fully reviewed (`410b47409b6`) and the current head (`d98cfb6085`): ``` diff --git a/.../TaskDeployStaleContextRaceTest.java - private static final long DEPLOY_TIMEOUT_SECONDS = 10; + private static final long DEPLOY_TIMEOUT_SECONDS = 30; - private static final int ITERATIONS = 5; + private static final int ITERATIONS = 30; ``` That's the only change anywhere in the diff - `TaskExecutionService.java` and `TaskExecutionServiceTest.java` are byte-identical to `410b47409b6`, so my last full review's conclusion carries forward unmodified rather than needing a fresh pass. This closes Issue 4 exactly as promised: `ITERATIONS` back to 30 restores the race test's regression-detection power, and restoring `DEPLOY_TIMEOUT_SECONDS` too (which I'd said was optional) is a good call for the reason you gave - a slow-but-healthy deploy on a loaded runner shouldn't be able to report a false hang. I also re-checked CI on this exact head (fork run `33297052269`): all four `unit-test` legs pass, including the restored 30-iteration `TaskDeployStaleContextRaceTest`. The two failures on this run (`all-connectors-it-2`, `doris-connector-it`) are both connector-IT jobs outside the two files this PR touches - `doris-connector-it` in particular has already failed the same way on earlier heads of this PR for unrelated reasons, as y ou noted a few rounds back. On your question about whether @SEZ9's Issues 1 and 6 on #11757 apply here: yes, and I traced the current code to confirm exactly what you described. `finishOwnedResources()` (`TaskExecutionService.java:1555`) calls `recycleClassLoader(taskGroupLocation, ownedContext)` unconditionally at line 1570, before the `!contextOwned` check - so in the stale branch, this tracker's own class loader gets recycled regardless of whether its own async-function/timer-flush entries (registered under the same reused `taskGroupLocation`) are still live and potentially still running against that class loader. That's the same underlying gap as Issue 2 in my last full review (carried from @SEZ9's Issue 4 on #11757: the stale branch never cancels its own async-function/timer-flush entries), just described from the other direction - not a new, independent defect. I checked how much this actually bites in practice: `DefaultClassLoaderService.releaseClassLoader()` (`seatunnel-engine-core/.../DefaultClassLoaderService.java:102-121`) decrements the reference count and returns immediately when `cacheMode` is true (`ServerConfigOptions.CLASSLOADER_CACHE_MODE` defaults to `true`) - it never removes the class loader from `classLoaderMap` or calls `recycleClassLoaderFromThread()` in that branch. So under the default configuration, an orphaned async/timer task racing this ordering keeps working off the still-live, still-cached class loader; nothing gets pulled out from under it. The real exposure is scoped to `classloader-cache-mode=false`, and only when the reference count from this call happens to hit exactly zero - the same conditional severity that's already been established for the sibling issues in this thread. Given that, I don't think this needs to be a new blocker on top of what's already tracked, and I don't think it needs fixing twice. My preference: leave it exactly where Issue 2 already sits in my last review - a Medium, non-blocking follow-up - and let it land in whichever of #11727 / #11757 gets there first, same as the coordination approach we already settled on for the ownership-model work itself. If you'd rather close it here since `finishOwnedResources()` already lives in this PR, that's fine too; either way, please don't block on my account. Net: no change to my conclusion. **Ready to merge** - the one open ask from my last review (the iteration-count restore) is done and verified in the diff, CI is green on everything this PR actually touches, and the remaining Issues 1-3 stay as accepted non-blocking follow-ups. As before, I'm a comment-only reviewer here, so a maintainer with write access (@davidzollo / @zhangshenghang) would still need to do the actual approve/merge step. -- 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]
