SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6406761858
@JeremyXin Thanks for the detailed walk-through — that answers my question. **handleSaveMode / JobMaster.init guard** — Agreed on keeping the non-restore guard in `JobMaster.init`; relaxing it would change existing cluster-side save-mode behavior, which is out of scope here. Framing the restore branch of `handleSaveMode(..., RestoreMode)` as preserved behavior for the public helper and the deprecated boolean overload (rather than something the guarded init path reaches) resolves my concern. One small ask: please put that note in the Javadoc of the new overload so the next reader doesn't assume the restore branch is exercised from `init`. **CheckpointCoordinator rename** — Confirmed. Since JobMaster already passes `jobImmutableInformation.isRestoreJob() || restart` through to the CheckpointCoordinator constructor, this is a pure name correction with no change to the value passed. I'm withdrawing the "call site not updated" point. Could you add that one-sentence call-chain explanation to the PR description so the rename is documented for future readers? **Still open** — The one item not covered in your comment: checkpoint-mode restore submissions that hit a pending cleanup record or existing job metrics now fail with `JobException` where they previously took the savepoint path. I'm fine with the behavior itself, but it's a user-facing change on upgrade, so please mention it in the PR description (and release notes if applicable). Once that's in, I have nothing further on this PR. <!-- streview-comment:2034 --> -- 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]
