SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6594126327
Thanks for the update. The delta between `881e275f` and `c1cb6137` is only a `dev` merge, so the hunks I looked at earlier are unchanged. The latest review describes this as a behavior-preserving refactor, so I may have misread parts of the chain — a few clarifications would help me close out my earlier points: 1. **Legacy payloads** — for a payload from an older client where `restoreMode` is null but `isStartWithSavePoint` is true, how is it classified in `JobImmutableInformation` and `CoordinatorService.submitJob`? If existing tests in this PR already cover that path, a pointer is enough. 2. **Null handling in `submitJob`** — `restoreMode` looked null-guarded for the boolean derivation and then dereferenced right after. Can `restoreMode` actually be null there? If so, normalising it to `NONE` once up front would be simplest. 3. **`ClientJobProxy` wire boolean** — if the boolean now carries `isSavepointRestore()`, how does an old master handle a `CHECKPOINT` restore submitted by a new client during a mixed-version rolling upgrade? A sentence in the PR description would be enough. 4. **`JobMaster.handleSaveMode`** — the restore-aware branch's call site appeared to be reachable only for non-restore jobs. Is that branch reachable, and if not, where do CLUSTER-side restore jobs get schema save-mode handling? 5. **`CheckpointCoordinator` `isRestoreJob` rename** — which call site passes the new semantics? I may have missed it. 6. **Checkpoint-mode restore hitting a pending cleanup record or existing job metrics now throws `JobException`** — if intended, a short note in the PR description on the user-visible change would be helpful. Happy to re-review once these are clarified. <!-- streview-comment:2241 --> -- 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]
