SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-5738681603
@JeremyXin Thanks for laying out the two options so clearly — I agree we should not end up with two competing restore decisions inside `CoordinatorService.submitJob`. My recommendation: keep Option 1 in this PR and defer the wire-level removal to a follow-up. Concretely: 1. Keep the `isStartWithSavePoint` parameter on `submitJob(...)` / `SubmitJobOperation` purely for wire compatibility and mark it deprecated with a short note that it is no longer consulted on the master side. Removing it from the protocol is a bigger compatibility conversation and deserves its own change. 2. Make `JobImmutableInformation` the single source of truth, but close the legacy gap there: when the deserialized `restoreMode` is null, fall back to the legacy `isStartWithSavePoint` field (true → savepoint restore, otherwise no restore). With that fallback, an old client's payload resolves correctly and there is no need to compare the wire boolean against the payload — that gives you the single decision path you want. 3. In `CoordinatorService.submitJob`, derive both the restore decision and the savepoint-specific branch (pending cleanup record / existing metrics handling) from the resolved restore mode, and drop the mixed null-guard / unconditional-dereference pattern — once the fallback in (2) exists, the resolved mode is never null and the code can say so plainly. 4. On the client side, since old masters still branch on the wire boolean, `ClientJobProxy` should keep sending true only for a savepoint restore (not for checkpoint restore), so a new client against an old master behaves as before. Two remaining asks from the earlier points, independent of the option choice: - `handleSaveMode(SeaTunnelSink, RestoreMode)` in `JobMaster` is only reached under `!jobImmutableInformation.isRestoreJob()`, so its restore-aware branch is unreachable and cluster-side restore jobs currently get no schema save-mode handling. Please either move the call outside that guard or simplify the method, and add a test covering a restore job. - The `CheckpointCoordinator` parameter was renamed to `isRestoreJob`, but I could not find a call site updated in this PR to pass the checkpoint-restore case. Could you point me to where the caller now passes the resolved restore flag, or add that change? It would also help to add a short note in the PR description about the intentional behavior change for checkpoint-mode restores that hit a pending cleanup record or existing metrics (now a `JobException` instead of following the savepoint path), so it makes it into the release notes. <!-- streview-comment:1154 --> -- 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]
