DanielLeens commented on PR #12271:
URL: https://github.com/apache/seatunnel/pull/12271#issuecomment-6143590417
@SEZ9 Thanks for the follow-up. I re-read `TaskExecutionService.java` on
head `40c6fab85` (still the head, still a pure sync with `dev`), so these are
answered against that exact commit.
**F1 - where the hunk is, and what a recycled context returns.**
- `taskDone` -> `finishExecution(...)` ->
`recycleClassLoader(taskGroupLocation, context)` on the tracker's own context
is at `TaskExecutionService.java:1591-1599`, `:1664-1694` and `:1635-1641`; the
`context.equals(activeContext)` guard sits inside `executionContexts.compute`.
- If the tracker's own context has already been recycled,
`TaskGroupExecutionTracker.getTaskClassLoader` (`:1643-1655`) does not return
`null`: the loader map is `null` at that point and the method throws
`IllegalStateException("Classloaders for task group ... have already been
recycled")`. So that case already fails fast and is not silently installed as a
null TCCL.
- The one remaining `null` is the case where the map exists but has no entry
for that task id (a task group deployed without connector jars). That is the
same behavior as before this PR (the old code did
`executionContexts.get(location).getClassLoaders().get(taskId)`), so I treated
it as unchanged semantics rather than something this PR should alter. A WARN
there would be a fair optional improvement, but I would not block on it.
**F2 - `BlockingWorker`.** Confirmed. `BlockingWorker.run()` calls
`startedLatch.countDown()` first (`:1284`) and then resolves the loader through
`taskGroupExecutionTracker.getTaskClassLoader(t.getTaskID())` (`:1287-1288`);
it no longer reads `executionContexts`. `CooperativeTaskWorker.run()` does the
same at `:1421`. So non-cooperative tasks get the same fix, and the latch
ordering that #12454 relies on is preserved.
**F3 - tests.**
`TaskExecutionServiceTest#testStaleGenerationCleanupDoesNotRecycleNewerContext`
(`:615`) publishes a newer context at the shared `TaskGroupLocation`, runs
`firstTracker.taskDone(...)`, and asserts the first context's loaders are
nulled while the newer one stays published and resolvable.
`#testCooperativeTrackerKeepsGenerationSpecificClassLoader` (`:573`) checks
that each generation's tracker returns its own loader. A test that drives the
real worker `run()` loops is still not there; as you said, that is a fine
follow-up and I keep it non-blocking.
CI on this head: the apache-side `Build` check is `SUCCESS` (fork run
`37393279489`, attempt 2). My conclusion is unchanged: no source-side blockers,
and a committer's final approval is the remaining step. Thanks both for the
careful look!
--
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]
