SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-5747498722
Following up on my previous comment to make the remaining scope clear. **Withdrawn: F1 / F2 / F4 / F5 / F7.** As traced above against the pinned head `e0b46feedf`, the net change in `CoordinatorService.submitJob` is comments only, the coordinator logic is unchanged, and every released producer already sends the savepoint-only boolean, so the `isSavepointRestore()` switch is a value-preserving rename and legacy payloads without a trailer still derive `SAVEPOINT`/`NONE` from the legacy field. F4 and F7 were measured against the earlier revision `7f55adf8`, the same mistake I acknowledged for F1/F2, and the code they describe is not on the head under review. Please keep Option 1 as is. **Still open** - **F3 (JobMaster)** – The restore-aware branch of the new `handleSaveMode(SeaTunnelSink, RestoreMode)` looks unreachable: its only call site is guarded by `!jobImmutableInformation.isRestoreJob()`, so `restoreMode` is always `NONE` there and cluster-side restore jobs get no schema save-mode handling. Either move the guard into the method so the restore path runs, or drop the dead branch and describe the intended behavior for restored jobs. - **F6 / F8 (CheckpointCoordinator)** – The parameter was renamed to `isRestoreJob`, but I don't see a constructor call site updated in this PR. Could you point me to where the checkpoint-restore case now passes `true`, or update the call site so the rename actually changes behavior? Happy to re-review promptly once the next commit lands. <!-- streview-comment:1189 --> -- 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]
