DanielLeens commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6397029281
Thanks @SEZ9 for the re-trace and for withdrawing the compatibility and `CoordinatorService.submitJob` points. I looked at both remaining items on head `881e275f`. 1. **`JobMaster.handleSaveMode` reachability** - You read it correctly: the call site in `JobMaster.init` is guarded by `!restart && !jobImmutableInformation.isRestoreJob()`, so on that path `getRestoreMode()` is always `NONE`. The guard now carries a comment saying restore jobs intentionally skip cluster-side save-mode handling, so this keeps the pre-existing behavior rather than changing it. The `RestoreMode` overload and the deprecated boolean overload are `public static`, so they remain as API surface. I think it is fine to leave as is. @JeremyXin, please confirm that no other caller passes a restore mode into it. If there is none, a short note to that effect is enough. 2. **`CheckpointCoordinator` `isRestoreJob`** - In this PR the change in `CheckpointCoordinator` is only the constructor parameter rename (`isStartWithSavePoint` -> `isRestoreJob`, around lines 219 and 283). The value comes from the unchanged-signature chain `JobMaster.initCheckPointManager`, which passes `jobImmutableInformation.isRestoreJob() || restart` (`JobMaster.java:343`) into `CheckpointManager` and on to the coordinator. So no constructor call site needs updating, and the "not a new job" branch now also covers non-savepoint restores, as intended. Both look good to me; I will re-check once any follow-up is pushed. (Reviewed by code reading only; nothing was executed locally.) -- 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]
