davidzollo commented on PR #11382: URL: https://github.com/apache/seatunnel/pull/11382#issuecomment-5579163445
Pushed `1ee22aced02` with the test and the comment, plus a source-traced answer to the coordinator-not-recreated question. - **Restore-path test** — added `testAllTaskReadyRoutesRestoredSchemaChangeCheckpointThroughNotifyCompleted` in `CheckpointCoordinatorTest`, built on the existing `buildMinimalCoordinator`/spy harness this file already uses for `allTaskReady()` regression tests. It sets `latestCompletedCheckpoint` to a restored `SCHEMA_CHANGE_BEFORE_POINT_TYPE` checkpoint, drives all pipeline subtasks to `READY_START`, invokes `allTaskReady()` via reflection, and asserts (in order) that `notifyTaskStart()` runs before `notifyCompleted()` is called with that exact `CompletedCheckpoint` instance. `notifyCompleted()` itself is stubbed since it dispatches to remote member nodes that a single-process unit test can't observe end-to-end — the property this test pins down is specifically that `allTaskReady()` performs the call, with the type intact, in the right order. - **Comment** — added to `SchemaChangeDrainGuard.checkpointCompleted(long, CheckpointType)`'s Javadoc, spelling out that this is the only call that can reopen a freshly constructed guard, that it's driven by `allTaskReady()`'s replay, and why `isAllTaskReady`'s once-per-coordinator guard can't suppress it for a task that needs it. - **Coordinator-not-recreated question** — traced this rather than guessed: `SubPlan.cancelPipeline()` calls `cancelCheckpointCoordinator()` (`jobMaster.getCheckpointManager().cancelCheckpoint(pipelineId)`) before a pipeline `reset()`, and the comment on `reset()` states "Before restore a pipeline, the pipeline must do reset" — both operate at pipeline granularity, not per-task. `CheckpointManager` builds exactly one `CheckpointCoordinator` per pipeline plan in its own constructor, and `CheckpointManager` itself is (re)constructed once per job/restore attempt. So a task restart in this engine is always pipeline-scoped and always goes through the cancel → reset → recreate cycle, which recreates the `CheckpointCoordinator` (and therefore resets `isAllTaskReady`) every time. The "coordinator survives while one task restarts" scenario isn't reachable here — I don't see a path where a task's `SinkFlowLifeCycle`/`SchemaChangeDrainGuard` gets rebuilt without its pipeline's coor dinator also rebuilding alongside it. `./mvnw spotless:apply` run on `seatunnel-engine-server` before pushing, no further formatting changes produced. Per the SeaTunnel local-verification rule I'm not running the module's tests locally — GitHub CI on this head is the verification source of truth. -- 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]
