SEZ9 commented on PR #11382: URL: https://github.com/apache/seatunnel/pull/11382#issuecomment-5578001874
@DanielLeens thanks for tracing this end-to-end at `634a0325a023` — that answers my F2 concern, and I agree with the mechanism as you describe it. What I had missed is exactly what you point out: `allTaskReady()` unconditionally calling `notifyCompleted(latestCompletedCheckpoint)` once every subtask reports `READY_START`, with `latestCompletedCheckpoint` restored from the persisted `PipelineState` in the coordinator constructor. Since `notifyCheckpointCompleted` fans out to every subtask in `plan.getPipelineSubtasks()` and the guard's `isSchemaChangeBeforeCheckpoint(id, type)` keys only on the type, the replayed BEFORE-type notification does open the freshly-restored guard before the source re-emits the `SchemaChangeEvent`. I was reading only the diff and didn't account for this pre-existing re-delivery path, so I'm withdrawing F2 as a blocking bug. Two things I'd still like before closing it out, since the fix relies on this behaviour rather than anything the PR adds: 1. A test that pins the restore path down (this also covers the "recovery depends entirely on notification re-delivery" point in F4 and the wiring gap in F7): start from a state where `latestCompletedCheckpoint` is a schema-change-before checkpoint, bring all tasks to `READY_START`, and assert the restored `SchemaChangeDrainGuard` has its drain window open before a `SchemaChangeEvent` is delivered. Right now nothing in the test suite would catch a future change to `allTaskReady()` silently breaking this. 2. A short comment in `SchemaChangeDrainGuard` (or next to `checkpointCompleted`) noting that recovery of the drain-ready flag depends on the coordinator's `allTaskReady()` replay of `latestCompletedCheckpoint`. Anyone reading the guard in isolation, as I did, will hit the same question. Could you also confirm the case where the coordinator is *not* recreated (task-level restart while the coordinator instance survives)? From your description `latestCompletedCheckpoint` would still be held in memory and replayed via `allTaskReady()`, but I'd like that stated explicitly rather than assumed. With those, I'm happy to mark F2 resolved alongside F1. <!-- streview-comment:874 --> -- 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]
