DanielLeens commented on PR #11727: URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5412481023
@abdessalems - good question, and thanks for holding off rather than guessing. I looked into #11757 properly rather than answering from memory, since I've been reviewing it in parallel and wanted to give you a grounded answer. Recommendation: land #11727 with the narrower change only (the `getClassLoaders()` null guard in `BlockingWorker.run()`, which you said is already written) and leave the cross-generation eviction fix entirely to #11757. Don't duplicate the ownership-check logic here. Why: #11757 (@waterWang) already implements exactly the ownership model you're describing - `TaskGroupExecutionTracker` binds the `TaskGroupContext` it owns as a `final` field at construction, and `taskDone()`'s teardown does an identity-checked, atomic ownership-check-then-teardown under `synchronized (TaskExecutionService.this)` (the same monitor `deployTask()` already holds around `deployLocalTask()`). It's more complete than a `remove(key, value)` alone would be, since it also gates cleanup of `cancellationFutures`, async-function futures, and timer-flush futures on the same ownership check, not just the execution context. Writing the identity check again here would either duplicate that work or risk diverging from it in a way that creates a real merge conflict once #11757 lands. #11757 is close: the design itself is confirmed sound across three independent review rounds (object-identity ownership rather than a generation counter sidesteps a whole class of off-by-one bugs). The one remaining blocker is narrow and already diagnosed: the stale/rejected branch of `finishOwnedResources()` returns without ever calling `recycleClassLoader(taskGroupLocation, ownedContext)`, so every time the guard actually fires, that generation's class loader is never released - a real leak under `classloader-cache-mode=false`, though a no-op under the default cache mode. The fix is a one-line addition before the `return` in that branch, and multiple reviewers now agree on the exact location and fix. One correction to my own position: when I raised the eviction race as Issues 1/2 on this PR on 08-22, I should have cross-referenced #11757 explicitly instead of describing the fix in the abstract - I already had review history there by that point and didn't connect the two clearly enough, which contributed to the back-and-forth since. To be clear now: that eviction/ownership work belongs to #11757, not #11727. So, concretely: push the `getClassLoaders()` guard here, hold the eviction change and its cross-generation test out of this PR, and once #11757's one remaining blocker lands, this PR and #11757 can merge independently in either order - #11727's guard doesn't depend on #11757's fix, and #11757's fix doesn't depend on this PR's guard being present. I'll do a fresh, focused pass on just the `getClassLoaders()` guard once you push it. -- 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]
