zhangshenghang commented on PR #12271:
URL: https://github.com/apache/seatunnel/pull/12271#issuecomment-5645668072

   Thanks @SEZ9 for the thorough review, and @DanielLeens for verifying the 
findings — all three issues are addressed in 6202ad987:
   
   - **Issue 1 (cleanup generation-awareness)**: `recycleClassLoader()` now 
operates on the tracker's `ownedContext` (nulls its own loader map and releases 
its own jars) instead of resolving through the location-keyed map. `taskDone` 
uses `executionContexts.remove(taskGroupLocation, ownedContext)` so a stale 
generation can no longer remove or recycle a newer generation's context, and 
the finished map now records the generation that actually finished.
   - **Fail-fast on recycled context**: `getTaskClassLoader` now throws 
`IllegalStateException` when the owned context's loader map has been recycled 
(null), instead of silently handing a null TCCL to the worker. Note the null 
check is on the loader **map**, not the per-task entry: a task without a 
registered per-task loader (task groups deployed without connector jars) still 
falls back to the system TCCL exactly as before, so no behavior change for that 
legitimate case.
   - **Issue 2 (BlockingWorker)**: `BlockingWorker.run` now resolves via 
`getTaskClassLoader` as well, and the resolution moved inside the try-block 
right after `startedLatch.countDown()`, so a lookup failure can no longer leave 
the deployer blocked on the latch.
   - **Issue 3 (regression test)**: added 
`testStaleGenerationCleanupDoesNotRecycleNewerContext` — publishes two 
generations at the same `TaskGroupLocation`, runs the stale generation's 
`taskDone`, and asserts the newer generation's context stays published with its 
classloaders intact (this fails against the previous location-keyed cleanup). A 
fuller end-to-end regression through `deployLocalTask` + 
`CooperativeTaskWorker` is a fair follow-up; I kept this one to the unit level 
to stay reviewable.
   
   Local verification: `TaskExecutionServiceTest` 17/17 green including the new 
test and `testThrowException`.


-- 
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]

Reply via email to