DanielLeens commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6266164534
Thanks @JeremyXin for the six-point rundown — that's precisely what made this re-review quick, and it lines up with what I traced in my own pass. My read on each: 1. **Legacy payload compatibility** — agreed. `JobImmutableInformation.readData()` initialising `restoreMode` from the legacy boolean before reading the optional trailer (`JobImmutableInformation.java:259–268`) means a pre-`RestoreMode` payload with `isStartWithSavePoint == true` still resolves to `SAVEPOINT`, so `SaveMode` can't wipe data on the legacy client path. `shouldReadLegacyPayload` pins it. Closed. 2. **Wire boolean semantics** — agreed, and this is the part I'd most want pinned down. `ClientJobProxy` now sends `isSavepointRestore()`, but since that is derived from the same `RestoreMode` on constructed payloads, the value on the wire is unchanged (`JobImmutableInformation.java:117–119`) and the field layout is untouched. So Option 1 stays the right call here: no mixed-version hazard introduced, and retiring the legacy boolean from the wire remains a separate, explicitly-flagged change with its own deprecation window. 3. **`handleSaveMode(SeaTunnelSink, RestoreMode)` reachability** — this is the one I'd like to see tracked rather than just noted. You're right that the restore-aware branch is unreachable through `JobMaster.init` because the guard already skips restore jobs, so today the new `RestoreMode` parameter is effectively dead at that call site. You're also right that changing cluster-side sink initialization is a separate behaviour decision that needs its own regression test. My ask is small: a tracking issue link in the thread or the PR description, so the next person reading `handleSaveMode` doesn't assume the parameter is live. If the method is intentionally left for the follow-up, a one-line Javadoc saying "restore-aware branch is not reachable from `JobMaster.init` yet" would prevent the same question being re-litigated. 4. **Checkpoint-mode hard-fail** — thanks for correcting the framing. The `CoordinatorService.submitJob` pending-cleanup and existing-metrics branches are unchanged by this diff; a comment-only touch. Filling in the "user-facing change" and "How was this patch tested?" sections with the actual current behaviour is the right call, and explicitly *not* presenting it as a new breaking change is more accurate than the earlier wording. 5. **`CheckpointCoordinator` rename** — `JobMaster.initCheckPointManager` passing `isRestoreJob() || restart` (`JobMaster.java:342`) and forwarding through `CheckpointManager.java:149–160` matches what I traced. Closed. 6. **Null handling in `submitJob`** — confirmed, no null-guard-then-dereference on the current head; the mode is resolved during legacy deserialization before any downstream use. So the source side looks good to me on `c1c542de8cc8`, and the head's `Build` check is green. What's left is the two PR-description sections from @SEZ9's earlier note (non-blocking, but they do feed the release notes, so worth filling in) and the point-3 tracking note above. @JeremyXin this has been a long thread and you've handled every item carefully — thanks for staying on it. `mergeStateStatus` here is `BLOCKED` only on the missing qualifying approval, so a maintainer with write access can take the final step; I don't have write-level review controls on this repository. -- 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]
