DanielLeens commented on PR #12271:
URL: https://github.com/apache/seatunnel/pull/12271#issuecomment-5678497951
@SEZ9 — good timing question, I posted a full re-review of `80f6c5e44ac`
about 12 minutes before your comment, so here are the direct pointers rather
than making you dig for them:
**F1 (location-keyed terminal cleanup)** — confirmed present in the merge,
now expressed through `dev`'s `#12238` primitives rather than the original
`remove(location, ownedContext)` shape:
- `getTaskClassLoader()` fail-fast: `TaskExecutionService.java:1643-1655` —
throws `IllegalStateException` naming the `TaskGroupLocation` when
`context.getClassLoaders() == null` (recycled), returns
`classLoaders.get(taskId)` otherwise (legitimate `null` only for a task with no
per-task loader, no behavior change there).
- Identity-checked cleanup: `TaskGroupExecutionTracker.taskDone()` →
`finishExecution(location)` at `:1599, 1664-1704` —
`executionContexts.compute(location, (ignored, active) ->
context.equals(active) ? {...; return null} : active)` at `:1671-1679`.
Identity is via `TaskGroupContext.equals()`/`hashCode()`, which is
`executionId`-only (see `execution/TaskGroupContext.java:98-113`) — so this is
a no-op against the map once a newer generation has overwritten the entry for
that location. `recycleClassLoader` operates on `context` (the tracker's own
object) at `:1694-1698`, never on `executionContexts.get(location)`.
**F2 (`BlockingWorker` location-keyed read)** — confirmed, yes it now
resolves off the tracker-owned context, not the shared map:
`BlockingWorker.run()`, `TaskExecutionService.java:1277-1300` —
`startedLatch.countDown()` runs first and unconditionally at `:1284`, then
`taskGroupExecutionTracker.getTaskClassLoader(t.getTaskID())` at `:1287-1288`,
both inside the same `try`. So the generation-safety fix and the latch-ordering
fix are both intact, and I also re-verified in my review that a thrown
`IllegalStateException` here still reaches
`taskGroupExecutionTracker.exception(e)` → `taskDone(...)` via the existing
`catch (Throwable e)`, not a silent worker exit.
**F3 (end-to-end regression coverage)** — still open, still non-blocking,
listed as Issue 1 in my last review's summary table. Your suggested shape
(publish two `TaskGroupContext` generations at the same location, drive an
older-generation task through the real worker, assert it resolves the older
loader while the newer context stays intact) is exactly the gap — the two
existing unit tests call `getTaskClassLoader()`/`taskDone()` directly on
hand-built trackers rather than through an actual
`CooperativeTaskWorker.run()`/`BlockingWorker.run()` iteration, so a regression
at `:1284`/`:1287`/`:1421` itself wouldn't be caught today. Fine as a follow-up
rather than blocking this PR.
With F1/F2 confirmed against the actual diff and F3 tracked as a
non-blocking follow-up, my conclusion is unchanged: Ready to merge, no open
blockers from my side.
--
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]
