SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-5724542584
Thanks for the detailed walkthrough, @JeremyXin. If the callers (`ClientJobProxy` and both `BaseService` submission paths) pass `jobImmutableInformation.isSavepointRestore()` and `CoordinatorService.submitJob` consumes that caller-supplied value instead of re-deriving it, that matches the direction we discussed, and I'm not asking for a revert. I'd still like to verify that in the diff before closing out the wire-boolean concern, so a pointer to those call sites in the current revision would help. A few items from the earlier review are still open: 1. **Legacy payloads with `restoreMode == null`.** Using the caller-supplied boolean in `submitJob` may cover the cleanup/job-metrics branches, but the restore-job classification in `JobImmutableInformation` is also used downstream (e.g. the `JobMaster` save-mode guard). How is a payload with `restoreMode == null` and `isStartWithSavePoint == true` classified there? If it is treated as a non-restore job, the `SaveMode` data-loss risk remains. A unit test covering that legacy shape would be ideal. 2. **Null handling in `submitJob`.** Please confirm there is no longer a null-guarded read of `restoreMode` followed by an unconditional dereference. 3. **Save-mode handling in `JobMaster`.** The restore-aware branch of the new save-mode method is only reachable when the job is not classified as a restore job, so it can never see a real restore mode, and cluster-side restore jobs get no schema save-mode handling. Either drop the unreachable branch or move the call so restore jobs pass through it. 4. **`CheckpointCoordinator` `isRestoreJob` parameter.** The rename is in, but I don't see the constructor call site updated in this PR. Could you point me to it, or update it so checkpoint restores pass the restore flag rather than the savepoint-only one? 5. **Behavior change documentation.** Checkpoint-mode restores that hit a pending cleanup record or existing job metrics now fail with `JobException` instead of following the savepoint path. That's fine as an intentional change, but please note it in the PR description / release notes. Once those are addressed I'll do a final pass. <!-- streview-comment:1137 --> -- 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]
