DanielLeens commented on PR #11382: URL: https://github.com/apache/seatunnel/pull/11382#issuecomment-5611975763
@SEZ9 Reposting the correction as a standalone comment since the collapse hit it again — same finding as Issue 1 in my formal review on `1ee22aced02` (https://github.com/apache/seatunnel/pull/11382#pullrequestreview-5148573950), pasted on its own this time: davidzollo's claim that the "coordinator survives while one task restarts" scenario "isn't reachable" doesn't hold up against the actual wiring: - `CheckpointManager.coordinatorMap` (`CheckpointManager.java:84, 119-149`) is populated exactly once, in the `CheckpointManager` constructor — one `CheckpointCoordinator` instance per pipeline for the life of that `CheckpointManager`. - `SubPlan.cancelPipeline()` -> `cancelCheckpointCoordinator()` -> `CheckpointManager.cancelCheckpoint(pipelineId)` (`CheckpointManager.java:397-398`) looks up and calls a method on the *existing* coordinator; it doesn't remove or replace the map entry. - `SubPlan.prepareRestorePipeline()` (`SubPlan.java:489-515`, the ordinary task-restart path, not master failover) -> `reportedPipelineRunning(pipelineId, false)` (`CheckpointManager.java:208-212`) resolves that same existing coordinator and calls `restoreCoordinator(false)` on it. So the coordinator instance is **not** recreated on an ordinary task-level restart. What actually resets the replay latch is `restoreCoordinator(false)`'s `isAllTaskReady.set(false)` (`CheckpointCoordinator.java:673, 707`) on the *surviving* coordinator — that's what re-arms `allTaskReady()`'s one-shot `notifyCompleted(latestCompletedCheckpoint)` replay for the freshly recreated sink task's fresh guard. The fix's actual runtime behavior is correct either way; only the javadoc's stated reason ("coordinator always recreated") is wrong, and I'd like it corrected before merge since it's the explanation future maintainers will trust. On your two follow-ups: 1. **F1 interaction with the replay** — confirmed directly from `SchemaChangeDrainGuard.java`: there's no separate code path for "first completion" vs. "replayed completion." `checkpointCompleted(id, type)` is the single synchronized entry point either way and calls `notifyAll()` after setting `schemaChangeDrainReady=true`; `checkSchemaChangeCanApply()` is synchronized on the same guard object and does a bounded `wait()` while `schemaChangeDrainReady` is false. So a fresh guard that sees the replayed `SchemaChangeEvent` before the replayed `CheckpointFinishedOperation` lands just blocks in the identical wait/notify pairing F1 already fixed — it's the same mechanism, only triggered by `allTaskReady()`'s replay call instead of the coordinator's first-time fan-out. 2. **Task-restart-only test** — checked: `testAllTaskReadyRoutesRestoredSchemaChangeCheckpointThroughNotifyCompleted` pins the `CheckpointCoordinator`-level mechanism (`allTaskReady()` calls `notifyCompleted(latestCompletedCheckpoint)`, in order, after `notifyTaskStart()`), but doesn't go further down into a fresh `SchemaChangeDrainGuard` actually flipping to ready and a queued DDL event being accepted afterward. That gap is real. Since (1) shows the guard-level half is the same wait/notify pairing `SchemaChangeDrainGuardTest` already covers, and the coordinator-level replay is now pinned separately, I'm comfortable keeping this a non-blocking recommended follow-up rather than a new blocker — consistent with how I scored it in the formal review — but a fast-follow integration test connecting the two halves explicitly would be welcome. Net: I'd treat F2/F4 as resolved on the current head — both restart shapes (coordinator-recreated and coordinator-survives) replay through the same `notifyCompleted(latestCompletedCheckpoint)` call, and F1's wait/notify covers the replayed-notification race the same way it covers the first-time one. The javadoc correction above (Issue 1) is the one item I'd still like fixed before merge. -- 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]
