SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-5358312927
Thanks for the thorough review and the approval, @DanielLeens — your summary matches the intent of the change exactly. A few notes on the points you called out: - **`CoordinatorService.submitJob`**: agreed that deriving the savepoint semantics from the deserialized `JobImmutableInformation` (via the new `isSavepointRestore()` / `isCheckpointRestore()` accessors) is the key behavioral piece. The RPC-level `boolean isStartWithSavePoint` stays on the wire purely for compatibility, but it is no longer trusted for the cleanup / pending-cleanup / job-metrics branches — the unit test covering the case where the wire boolean disagrees with the payload's real restore mode is there to lock that in. - **`JobMaster.init` / `handleSaveMode`**: switching the signature to take `JobImmutableInformation` / `RestoreMode` directly removes the pre-collapsed boolean, so a `CHECKPOINT` restore now correctly skips savemode re-execution the same way a savepoint restore does. - **`MultipleTableJobConfigParser` and the `CheckpointCoordinator` parameter rename** (`isStartWithSavePoint` -> `isRestoreJob`): both intentional — the parser needs the full `RestoreMode` rather than a boolean, and the coordinator parameter already carried "any restore" semantics, so the rename just makes the code honest about it. Two quick asks before merge: 1. Could you confirm you're comfortable keeping the legacy `isStartWithSavePoint` wire argument as-is for now (compatibility), with deprecation handled as a separate follow-up rather than in this PR? 2. Your review body appears cut off partway through the `CoordinatorService.java` before/after hunk — if you had an additional observation there, please post it so we don't lose it. Thanks again for the careful pass over the restore-mode unification. <!-- streview-comment:409 --> -- 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]
