DanielLeens commented on PR #11382:
URL: https://github.com/apache/seatunnel/pull/11382#issuecomment-5578497209
@SEZ9 thanks for tracing it through and withdrawing F2 — and for the two
follow-ups, both are worth doing.
**On your explicit question ("confirm the case where the coordinator is
*not* recreated"):** I traced this fresh rather than assuming it collapses to
the same path I described for F2, and it's actually a distinct branch worth
being precise about.
There are two different restart shapes in this codebase, and they take
different routes through `restoreCoordinator(boolean alreadyStarted)`:
1. **Task-level restart, coordinator instance survives**
(`SubPlan.prepareRestorePipeline()` at `SubPlan.java:498`, called on ordinary
pipeline failure/restore while the same JobMaster is alive): this calls
`reportedPipelineRunning(pipelineId, false)`, and
`getCheckpointCoordinator(pipelineId)` (`CheckpointManager.java:212`) looks the
coordinator up from the existing `coordinatorMap` — it is not reconstructed.
`restoreCoordinator(false)` (`CheckpointCoordinator.java:706-708`) only does
`isAllTaskReady.set(false)`; it never touches `latestCompletedCheckpoint`. That
field is the exact same in-memory object it was before the restart — nothing
re-deserializes it — because nothing in `cleanPendingCheckpoint` (`:1128-1131`,
called at `:681` during `restoreCoordinator`) resets it either; the only writes
to `latestCompletedCheckpoint` are the constructor (once, at coordinator
creation) and `notifyCheckpointCompleted`'s assignment (`:1330`) on each new
completion. So when the freshly
-restarted sink task (fresh `SchemaChangeDrainGuard`,
`schemaChangeDrainReady=false`) comes back up and reports `READY_START`, it
drives exactly the `allTaskReady()` ->
`notifyCompleted(latestCompletedCheckpoint)` replay I described for F2 — same
mechanism, and if anything a stronger guarantee here than the full-failover
case, since there's no deserialize-from-storage step at all.
2. **Master failover, coordinator recreated**
(`SubPlan.restorePipelineState()` at `SubPlan.java:601-604`): here
`alreadyStarted` is computed from whether every task's restored execution state
is already `RUNNING` (`SubPlan.java:582-599`) — i.e. the tasks themselves
survived the master flip and never restarted, only the master (and therefore a
brand-new `CheckpointCoordinator` object, rehydrated from persisted
`PipelineState` in the constructor, `:253-261`) did. In that case
`restoreCoordinator(true)` (`:695-699`) skips the `READY_START` fan-in entirely
and calls `notifyCompleted(latestCompletedCheckpoint)` directly — because the
already-running tasks won't emit a fresh `READY_START` for the coordinator to
react to, so the coordinator has to push the replay itself instead of waiting
for it.
Both branches end up calling the identical
`notifyCompleted(CompletedCheckpoint)` -> `notifyCheckpointCompleted` fan-out
to every subtask; they just differ in what triggers it (peer `READY_START`
reports vs. the coordinator's own explicit branch) and in whether
`latestCompletedCheckpoint` is the live pre-restart reference or a fresh
deserialize. So yes — in the task-level-restart-while-coordinator-survives
case, `latestCompletedCheckpoint` is held in memory (never reset) and is
replayed to the restored guard, exactly as you assumed, via the same
`allTaskReady()` mechanism.
On your two requested follow-ups, both are correct calls and line up with
Issue 6/7 in my last full review:
1. The restore-path regression test (pin `latestCompletedCheckpoint` to a
schema-change-before checkpoint, drive all tasks to `READY_START`, assert the
restored guard's window opens before the `SchemaChangeEvent` arrives) — agreed
this should exist rather than resting on review analysis of `allTaskReady()`.
2. A short comment on `SchemaChangeDrainGuard`/`checkpointCompleted`
documenting the dependency on the coordinator's `allTaskReady()` replay —
agreed, this is exactly the kind of implicit cross-class assumption that should
be spelled out for the next reader.
@davidzollo — with F2 withdrawn, I don't see any remaining blockers on the
source side; Issues 4-7 from my last review (multi-table guard scoping, abort
wiring, the restore-path IT, and now this doc comment) remain non-blocking
recommended follow-ups. One live fact worth flagging: `Build` is currently
showing `fail` on this head (`634a0325a023`) — that'll need to go green before
merge regardless of the review conclusion.
--
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]
