SEZ9 commented on PR #12271: URL: https://github.com/apache/seatunnel/pull/12271#issuecomment-5650172076
Thanks @zhangshenghang for the detailed writeup on 6202ad987. The approach you describe for each of the three points sounds like the right direction: - **F1**: having `recycleClassLoader()` act only on the tracker's `ownedContext`, using `executionContexts.remove(taskGroupLocation, ownedContext)` in `taskDone`, and recording the actually-finished generation in the finished map matches what I was after. Throwing `IllegalStateException` when the owned context's loader map has been recycled, while still falling back to the system TCCL when there is simply no per-task entry, is a sensible split — thanks for calling that distinction out. - **F2**: routing `BlockingWorker.run` through `getTaskClassLoader` and moving the lookup inside the try after `startedLatch.countDown()` should address both the generation issue and the latch concern. - **F3**: `testStaleGenerationCleanupDoesNotRecycleNewerContext` as described covers the cleanup path, and a fuller `deployLocalTask` + `CooperativeTaskWorker` run as a follow-up is fine with me. I still need to verify these against the actual diff before marking the findings resolved. In the meantime, two small asks: 1. For the fail-fast path: can you confirm (or add a short assertion) that when `getTaskClassLoader` throws inside `CooperativeTaskWorker.run` / `BlockingWorker.run`, the exception reaches the tracker's normal failure handling (i.e. the task group is marked failed rather than the worker exiting silently)? That was the "silent null" half of F1. 2. In the new test, please also assert on the stale tracker itself — that after its `taskDone`, calling its `getTaskClassLoader` throws `IllegalStateException`, and that the newer tracker's `getTaskClassLoader` still returns the expected loader. That pins both sides of the generation split in one place. If you'd rather fold the second one into the follow-up, just say so. I'll do a pass over the code once I've had a chance to read the diff. <!-- streview-comment:1002 --> -- 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]
