SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6258364244
Thanks @JeremyXin for the follow-up, I'll take another pass. To keep the re-review quick, could you point me to where each of the earlier points was addressed (or confirm if any is intentionally out of scope)? 1. **Legacy payload compatibility** (`JobImmutableInformation` / `CoordinatorService.submitJob`): how is a payload from an older client with `restoreMode == null` but `isStartWithSavePoint == true` handled now? It should still be treated as a savepoint restore so SaveMode doesn't wipe data. 2. **Wire boolean semantics** (`ClientJobProxy`): since the meaning of the boolean sent to the master changed, what happens in a mixed-version cluster where an old master still branches on it? If this is accepted as a breaking change, it should be documented. 3. **`handleSaveMode(SeaTunnelSink, RestoreMode)` reachability** (`JobMaster`): the restore-aware branch is only reached under `!jobImmutableInformation.isRestoreJob()`, so `restoreMode` is always `NONE` there. Is the intent that CLUSTER-side restore jobs skip schema save-mode handling entirely? If not, the guard needs adjusting. 4. **Checkpoint-mode restore hard-failing** with `JobException` on a pending cleanup record or existing job metrics: fine if intended, but please add a note to the docs/release notes since users upgrading will see different behavior. 5. **`CheckpointCoordinator` `isRestoreJob` rename**: which constructor call site now passes the restore flag instead of the savepoint-only flag, so checkpoint restores actually pick up the new semantics? 6. **Null handling in `submitJob`**: the `restoreMode` null guard followed by an unconditional dereference on the next line — has that been made consistent? If those are all covered, I'm happy to give the remaining +1. <!-- streview-comment:1857 --> -- 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]
