DanielLeens commented on PR #10551:
URL: https://github.com/apache/seatunnel/pull/10551#issuecomment-5601186076
Thanks @SEZ9. Same as on #10874 — I pulled my earlier comment back via the
API and it is not actually cut off; the sentence you quoted continues in full:
"...that's really the author's call to make, so I'll leave that to @davidzollo
to weigh in on rather than presuppose it from the review side." That's the
complete thought, nothing after it was dropped. Likely a rendering/collapse
artifact on a long comment, not a real truncation on my end.
One thing worth flagging before the F1/F8 answers: there's a newer commit on
this head than the one you were working from. `805608cd8aa1` (the F2 one-liner
you quoted) is not the current top commit anymore — `f6919c586496` landed after
it and is the current `headRefOid`. I did a full from-scratch re-review against
that exact head this morning (posted as a review, not just a comment) and it
changes the answer to both your questions:
**1. F1 — yes, already fixed, in `f6919c586496` (separate commit from the F2
one).** `repairMissingJobStateForRestore`'s fence
(`CoordinatorService.java:390`) now reads `|| getOwnedPendingCleanup(jobId,
jobInfo) != null` instead of the jobId-only `containsKey` check — I confirmed
this directly against the current head's diff, not just the PR description. It
ships with a dedicated regression test,
`testMissingStateRepairIgnoresStaleCleanupRecordFromForeignGeneration`, that
builds exactly the scenario you're asking about: a stale `JobCleanupRecord` at
`initializationTimestamp=100` left behind in `pendingJobCleanupIMap`, while the
currently-registered `JobInfo` is at `initializationTimestamp=200`, and asserts
`repairMissingJobStateForRestore` still succeeds (`JobStatus.CREATED`) and
clears the stale record rather than being blocked by it. So F1 is resolved and
tested, not open.
**2. F8 — depends which fence you mean, and the part you originally asked
about is still open.** I went and read the actual test diff in `805608cd8aa1`
rather than trusting the commit description: both rewritten
`CoordinatorServiceJobCleanupTest` cases
(`testSubmitStartWithSavePointConsumesOwnedPendingCleanupAndSucceeds` and the
`SavepointDoneState` variant) construct an *owned* cleanup record — same
generation as the restoring job — and assert the submission succeeds inline.
Neither constructs the unowned/foreign-generation case for that specific fence
(`submitJob`'s `validateJobSubmissionFence` savepoint-restart path). The new
`f6919c586496` commit's regression test I just described covers the
foreign-generation case, but for `repairMissingJobStateForRestore`, not for
`validateJobSubmissionFence`/`submitJob`. So: your original F8 concern
(unowned-record coverage for the savepoint-restart submit fence) is still an
open gap — I'd like a dedicated test there too, analogous
to the one just added for the repair path, before treating F8 as closed.
**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-merge 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.
--
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]
