DanielLeens commented on PR #12271:
URL: https://github.com/apache/seatunnel/pull/12271#issuecomment-5652838457
Confirmed both of your asks against the current head (`6202ad987`), for the
record:
1. Yes -- the `IllegalStateException` from `getTaskClassLoader()` is fully
routed through normal failure handling on both paths, not a silent worker exit.
In `CooperativeTaskWorker.run()` the call sits inside the `try` block (around
lines 1268-1274), and its `catch (Throwable e)` (line 1289) calls
`taskGroupExecutionTracker.exception(e)` then `taskDone(...)`, same as any
other task-body exception. In `BlockingWorker.run()` it's inside the `try`
right after `startedLatch.countDown()` (lines 1136-1140), and its own `catch
(Throwable e)` (line 1154) does the same `exception(e)` plus `finally {
taskDone(t) }`. `exception()` does a `compareAndSet` on `executionException`,
and `taskDone()` reads it back once the completion latch hits zero to drive the
task group to a failed terminal state -- so a recycled context now fails the
task group loudly on either path, which is exactly the fail-fast behavior the
fix is going for.
2. Your second ask -- asserting directly that the stale tracker's own
`getTaskClassLoader` throws after its `taskDone`, alongside the newer tracker
still resolving correctly -- is a good tightening of the existing test. It pins
both halves of the generation split in one assertion rather than only checking
the newer tracker's success and implying the stale one is inert. I'd be fine
with it landing in this PR or folded into the same follow-up as the end-to-end
`CooperativeTaskWorker`/`BlockingWorker` coverage we already agreed on; either
way it isn't blocking, since the current test already fails against the pre-fix
code for the scenario it does cover.
One thing worth surfacing that's independent of the code discussion: the PR
is currently showing `mergeStateStatus: DIRTY` / `mergeable: CONFLICTING`
against `dev`. That will need a rebase/merge from zhangshenghang before this
can actually be merged, regardless of where we land on the open questions above.
CI-wise, nothing has changed since my last pass: same fork run for this
exact head, same two `engine-v2-it` failures (`SplitClusterFaultToleranceIT`,
`BackpressureSlowSinkIT`) I already traced back to the pre-fix commit as
pre-existing, unrelated flakes -- no new failures to factor in.
My own conclusion stands at Ready to merge on the code itself; happy to
defer to your own pass over the diff before we call this fully closed.
--
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]
