DanielLeens commented on PR #11757: URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5451473080
Thanks for the update, @BinTaoMa, and thanks for carrying this forward with the original authorship preserved. I went and checked #11999 directly rather than taking the summary at face value, since the last blocking finding here (Issue 1 in my 2026-08-23T11:37Z review, independently rated High by @SEZ9 and Medium by me on 2026-08-19: the stale/rejected branch of `finishOwnedResources()` never called `recycleClassLoader(taskGroupLocation, ownedContext)` before returning) is exactly the kind of thing worth re-verifying rather than assuming fixed. I can confirm it: #11999's diff to `TaskExecutionService.java` adds `recycleClassLoader(taskGroupLocation, ownedContext);` right before the `return;` in the stale branch of `finishOwnedResources()`, alongside the existing call on the owned-context branch - so both branches now release the tracker's own class loaders regardless of which path is taken, closing the leak. The regression test (`testStaleTaskDoneDoesNotCleanupNewerGenerationResources`) was also extended with `Assertions.assertNull(oldContext.getClassLoaders())` after `oldTracker.taskDone( oldTask)`, which is a real assertion of the fix (not just "it ran without throwing") - it fails if that recycle call regresses again. Nice work closing this out precisely. On the overlap: since #11999 preserves this PR's commits/authorship and is explicitly framed as a continuation targeting the same root cause (stale `taskDone()` racing a newer restore generation's `deployLocalTask()`), I'd treat #11999 as the live PR for this fix going forward rather than reviewing both in parallel - it's already getting its own dedicated review pass in this batch, so I won't duplicate a full review of it here. This PR's head (`5a0a5def3dcb`) hasn't picked up any new commits, so per our usual process I'm not re-deriving a fresh full review on this PR itself; I'm logging this as awaiting a decision on whether #11757 gets superseded/closed in favor of #11999, or whether the fix gets cherry-picked back here - happy to formally re-review whichever branch ends up being the merge target once that's settled. -- 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]
