SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6392854916
Thanks for the update. Between `f38b5329` and `881e275f` the PR's own hunks look unchanged and the delta is a `dev` merge, so my remaining points are the same as before. Having re-traced the restore chain, I'm withdrawing the compatibility and `CoordinatorService.submitJob` concerns: the wire boolean (`isSavepointRestore()`) is the same predicate as the old `isStartWithSavePoint()`, and `submitJob` still branches on the caller-supplied flag, so I don't see a behavior change there. Two items I'd still like to settle: 1. **`JobMaster.handleSaveMode`** – its call site in `JobMaster.init` appears to be guarded by `!jobImmutableInformation.isRestoreJob()`, which would make the restore-aware branch unreachable (`restoreMode` would be `NONE` there). Could you confirm whether that is intended? If so, dropping the `RestoreMode` branch would be simpler; if not, the guard may need to move. 2. **`CheckpointCoordinator` `isRestoreJob` rename** – I couldn't find a constructor call site updated in this PR to pass `isRestoreJob()` instead of the savepoint-only flag. Could you point me to it, or add the change if it's missing? Otherwise the rename doesn't change behavior for checkpoint restores. Happy to take another pass once those are clarified. <!-- streview-comment:2015 --> -- 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]
