SEZ9 commented on PR #12271: URL: https://github.com/apache/seatunnel/pull/12271#issuecomment-6116887101
Thanks for the update. The thread notes that `40c6fab85` is a pure sync with `dev` and that the PR's own diff is unchanged from `3d57b64dd`, so my three earlier points still apply to this head. I can't verify the current code from the discussion alone, so a few confirmations would help me close them out: **F1 (terminal cleanup keyed by location)** – The description of `taskDone` going through `finishExecution` with a `context.equals(activeContext)` guard inside `executionContexts.compute`, followed by `recycleClassLoader(taskGroupLocation, context)` on the tracker's own context, is exactly the shape I was after. Could you point me at that hunk in the diff? Also, if a tracker's own context has already been recycled (loaders set to `null`), what does `getTaskClassLoader` return to the worker? If it can still return `null`, I'd prefer failing fast, or at least a WARN log, rather than silently installing a `null` TCCL. **F2 (`BlockingWorker` reading the location-keyed map)** – Please confirm that `BlockingWorker.run()` now resolves the loader via the tracker's `getTaskClassLoader` and no longer reads `executionContexts` directly, so non-cooperative tasks (`ThreadShareMode.OFF`, or `PART` with `isThreadsShare()==false`) get the same fix. The thread mentions `startedLatch.countDown()` still happens before the class-loader lookup, which is the ordering we want. **F3 (test coverage)** – `testStaleGenerationCleanupDoesNotRecycleNewerContext`, as described (publishing a newer context at the shared `TaskGroupLocation`, running `firstTracker.taskDone(...)`, and asserting the first context's loaders are nulled while the newer one stays published and resolvable), would address the core of my concern. Once I can see those assertions in the test diff I'm happy to close this. Non-blocking nit: a variant that drives `CooperativeTaskWorker` or `BlockingWorker` through `run()` would also lock in the call-site behavior; fine as a follow-up. CI on this head is reported green in the thread (fork run `37393279489`, attempt 2, succeeded). With the confirmations above I expect no further blockers from my side. <!-- streview-comment:1689 --> -- 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]
