SEZ9 commented on PR #11382: URL: https://github.com/apache/seatunnel/pull/11382#issuecomment-6008416991
Following up on the review items against `1ee22aced026`: **F5 — resolved.** `SinkFlowLifeCycle.notifyCheckpointAborted(long, CheckpointType)` now calls `schemaChangeDrainGuard.checkpointAborted(...)`, so the guard's abort-cleanup path has a real coordinator-driven caller. Nothing further needed. **F3 — confirmed as its own wire-format item, separate from F8.** In `CheckpointFinishedOperation`, `writeInternal` unconditionally appends `out.writeString(checkpointType.getName())` as a new trailing field, and `readInternal` unconditionally does `checkpointType = CheckpointType.fromName(in.readString())`. Mixed-version nodes exchanging this operation will therefore disagree on the format. Ask: either add a version guard around the new field, or make the read tolerant of a missing field with a sensible default, and document the compatibility expectation for rolling upgrades. **F8 — open; easy to address alongside F3.** `checkpointType.getName()` NPEs when `checkpointType` is null, and the deserialization path has no defensive handling for an unknown type name. Ask: null-check before serialization (or reject null in the constructor) and fail with a clear message or fall back to a default on unknown names. **F2 / F4 — open, to close together.** `schemaChangeDrainReady` remains guard-local in-memory state with no snapshot of its own. The coordinator's replay of `latestCompletedCheckpoint` on restart is what substitutes for persisted state, but that dependency currently exists only in review discussion. Ask: add javadoc on `SchemaChangeDrainGuard` (and ideally at the coordinator call site) stating that recovery of the drain-ready state relies on the completion notification being re-delivered from `latestCompletedCheckpoint` after restore. **F6 — open, unchanged.** `schemaChangeBeforeCheckpointId` is still a single `long`, so two overlapping schema-change-before/after pairs cannot be distinguished, and the type-based abort reset is broader than the specific checkpoint being aborted. Ask: track pending IDs keyed by checkpoint id and scope the abort reset to the matching id. If overlap is impossible by construction, please state the invariant in the code. **F7 — open, unchanged.** Current tests hit the guard directly or stub `notifyCompleted`. Ask: at minimum (a) a serialization round-trip test for `CheckpointFinishedOperation` covering the new field plus null and unknown-name cases (covers F3/F8), and (b) a test through the real `SinkFlowLifeCycle` -> `SeaTunnelTask` -> `CheckpointFinishedOperation` path. A schema-evolution-with-restart e2e would be ideal but can be an explicitly scoped follow-up. **F1 — no update yet.** Could you confirm whether the fail-fast guard no longer races with async checkpoint-completion delivery on the happy path and after restore, or point me to where that is handled? Summary: F5 done; F3 and F8 are the priority before merge; F2/F4 need the javadoc; F6 and F7 remain open; F1 needs a status update. <!-- streview-comment:1545 --> -- 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]
