SEZ9 commented on PR #11382: URL: https://github.com/apache/seatunnel/pull/11382#issuecomment-6459547624
Thanks for the update. I compared the current head `e442d3db` with `8858fc85` / `1ee22aced02`: the author changes in the 8 touched files are identical, and the only newer commit is a merge of `dev`. So the findings from the previous round still stand; consolidating them here as a single checklist: **HIGH** 1. **Fail-fast guard vs. async completion notification (`SchemaChangeDrainGuard`)** — `checkSchemaChangeCanApply` can be reached before the checkpoint-completion notification has been delivered to the sink subtask, so the happy path (and the post-restore path) can fail spuriously. The guard should tolerate the notification arriving after the barrier ack instead of treating "not yet ready" as a hard failure. 2. **No re-delivery of the typed completion after failover (`CheckpointCoordinator`)** — drain-ready state lives only in process memory and `CheckpointFinishedOperation` with the type is sent exactly once at checkpoint completion. After a restore from a schema-change-before checkpoint I cannot find the path that reopens the guard, which leads to a permanent DDL rejection loop. Please either point me to the re-delivery path or add one on the restore path. 3. **`CheckpointFinishedOperation` wire format change** — the new `CheckpointType` field is written/read without a version guard, and there is no null / unknown-name handling, so mixed-version clusters during a rolling upgrade can break on deserialization. **MEDIUM** 4. **Drain-ready not part of the sink snapshot (`SchemaChangeDrainGuard`)** — recovery relies entirely on notification re-delivery (see item 2). Snapshotting the state in the sink checkpoint would make recovery self-contained; at minimum the dependency should be made explicit and tested. 5. **Typed abort is unreachable (`CheckpointCoordinator`)** — only the completion path carries the type; no coordinator-side call site in this diff sends a typed abort, so the guard's abort cleanup never runs. Please either wire the abort path or remove the dead branch. 6. **Single-slot checkpoint tracking (`SchemaChangeDrainGuard`)** — overlapping schema changes overwrite each other's tracked checkpoint id, and the type-based abort reset clears state for unrelated checkpoints. 7. **Test coverage** — only `SchemaChangeDrainGuard` unit tests exist. Please add tests for the wiring (`SinkFlowLifeCycle`, `SeaTunnelTask` fan-out, `CheckpointFinishedOperation` serialization round-trip) and an end-to-end schema-evolution-with-failover case that restores from a schema-change-before checkpoint. 8. **NPE / unknown-name robustness on `checkpointType`** — serialization NPEs when `checkpointType` is null, and deserialization has no defensive handling for unknown enum names. This overlaps with item 3; a single fix can cover both. Suggested order: items 1–3 and 8 first, since they affect correctness on the normal path and upgrade safety. For items 2, 4 and 5, a short description of how the guard is reopened after restore would help me verify the fix quickly. Pushing the changes as new commits rather than a merge would keep the diff against `e442d3db` easy to review. Happy to re-review as soon as the updated commits are up. <!-- streview-comment:2086 --> -- 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]
