DanielLeens commented on PR #10551:
URL: https://github.com/apache/seatunnel/pull/10551#issuecomment-5611959707
Thanks @SEZ9. Confirmed via the raw API body again: my previous comment
(5601186076) is not cut off — the visible portion ending at "...covers the
foreign-generation case" is followed by roughly three more paragraphs, not
lost. Reposting that tail in full since you asked:
> **On F5** — confirmed, your read is correct: the
`jobMaster.init(...)`-still-under-lock hunk is unchanged by `f6919c586496`.
It's explicitly deferred with a documented rationale (removing it would drop
the mutual exclusion that prevents two concurrent restores from
double-constructing a `JobMaster`) rather than silently dropped — I agree
that's a larger, separately-reviewable change and it's reasonable to keep
deferred, but it needs to stay a tracked follow-up rather than fall out of
scope entirely once F1/F2/F8 land.
>
> One more thing to fold into your approval bar, since it postdates the
F1-F8 exchange and you may not have seen it yet: this morning's full review
also caught a new item — **Issue 1**, a genuine carryover I should have flagged
earlier and apologized for in the review itself.
`DistributedStateTransition.transition()` now folds a genuine
terminal-state-invariant violation (something trying to move a
job/task/pipeline backward out of a terminal state) into the same generic
race-handling branch as an ordinary, expected concurrent-race loss. Pre-PR this
threw a distinct `SeaTunnelEngineException` caught and logged at `ERROR` with
unique wording; post-PR it's indistinguishable in the logs from a benign race,
logged at `INFO` ("... because distributed state ... won the race"). Defensive
path only, no functional correctness break, but a real diagnosability
regression for exactly the kind of bug this check existed to surface — I've
rated it Medium and listed it as a blocker-before-mer
ge in the review (`DistributedStateTransition.java:1268-1271`, callers in
`PhysicalPlan.java`/`PhysicalVertex.java`/`SubPlan.java`).
>
> So the effective bar right now: F1 done+tested, F2 done, F4/F6/F7 done, F3
documented (partial, jobMaster.init hunk knowingly deferred), F5
unchanged/deferred as above, F8 partially addressed (repair-path covered,
submit-fence path still needs its own unowned-record test) — plus the new Issue
1 from this morning's review. Once F8's remaining gap and Issue 1 are addressed
(or Issue 1 gets an explicit maintainer call that the log-level tradeoff is
acceptable), I don't expect further structural concerns from me, and CI needs a
completed (non-cancelled) run on this head either way.
On your F8 split: agreed, that's exactly the right scoping — F1 closes the
`repairMissingJobStateForRestore` fence, F8 stays open specifically for the
`submitJob`/`validateJobSubmissionFence` savepoint-restart fence, and your
proposed test shape (unowned/foreign-generation `JobCleanupRecord`, same jobId,
through the submit path, asserting the submission isn't aborted) is the right
one to ask for.
Current bar to close, all on head `f6919c586496`: F8's submit-fence test,
plus Issue 1 (either fix the log level / exception type for the
terminal-state-invariant branch, or get an explicit maintainer sign-off that
folding it into the generic race path is acceptable).
--
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]
