SEZ9 commented on PR #11382: URL: https://github.com/apache/seatunnel/pull/11382#issuecomment-5629351892
Thanks for the detailed follow-up on the replay mechanism, and for reposting Issue 1 from the formal review on `1ee22aced02` (review 5148573950) as a standalone comment. **F2 / re-delivery path** — The walkthrough you describe (coordinator instance surviving an ordinary task-level restart, with `restoreCoordinator(false)` resetting `isAllTaskReady` to re-arm the one-shot `notifyCompleted(latestCompletedCheckpoint)` replay in `allTaskReady()`) is a plausible explanation, and I agree the javadoc should describe the `isAllTaskReady` reset rather than "coordinator always recreated" if that is what actually happens. However, I can only verify this against code, not prose. Could you point me to the commit that contains the replay change and the javadoc fix? I will keep F2 open until I have looked at the diff. **F1 / guard race** — Same request: the description of `checkpointCompleted(id, type)` as the single synchronized entry point that sets `schemaChangeDrainReady=true` and calls `notifyAll()`, paired with a bounded `wait()` in `checkSchemaChangeCanApply()`, sounds like the right shape for the fix. Please point me to the commit so I can confirm before marking F1 resolved. Remaining findings from my review that I have not seen a disposition for in this thread yet — please confirm status or push the changes: 1. **F3 / F8** — `CheckpointFinishedOperation` wire format: null-safe write of `checkpointType` and defensive handling of an unknown type name on read. 2. **F4** — Drain-ready state is process-memory only and not part of the sink checkpoint snapshot. If the position is that notification replay alone is sufficient, please say so explicitly so we can close this together with F2; otherwise let me know what the plan is. 3. **F5** — Is there a coordinator-side call site that sends a typed abort? If not, either wire one up or drop the unreachable typed-abort cleanup in the guard. 4. **F6** — Single-slot checkpoint tracking with overlapping schema changes, and the type-based abort reset being broader than the tracked checkpoint. 5. **F7** — At least one test covering the real wiring (`SinkFlowLifeCycle` → `SeaTunnelTask` fan-out → `CheckpointFinishedOperation` round-trip), ideally plus a schema-evolution-with-restart e2e case that exercises the replay path discussed above. Happy to re-review as soon as the commits are pushed. <!-- streview-comment:968 --> -- 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]
