SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6621488254
Follow-up on my earlier review, re-checked against head `c1cb6137` (the PR's own hunks are unchanged since `881e275f`): **Resolved / withdrawn** - **Legacy payloads (F1, F2):** `JobImmutableInformation.readData` seeds `restoreMode` from the legacy savepoint flag first (L261, flag -> `SAVEPOINT`, otherwise `NONE`) and only overrides it when the new trailer is present (L263), and `JobImmutableInformationCompatibilityTest` covers both legacy shapes. `CoordinatorService.submitJob` does not read `restoreMode` at all; it still branches on the caller-supplied boolean. Withdrawn. - **Null handling in `submitJob` (F7):** there is no `restoreMode` dereference in `submitJob(long, Data, boolean)`; the PR only adds a comment there (L1404-L1406). Normalization happens once in the `JobImmutableInformation` constructor (L117) and `isRestoreJob()` (L167) is null-safe on top of that. Withdrawn. - **Mixed-version wire boolean (F5):** `isSavepointRestore()` (L172) is true only for `SAVEPOINT`, which is exactly what the old `isStartWithSavePoint()` returned, and the `writeData`/`readData` and codec layouts are untouched. An old master sees the same bytes and takes the same branch. Withdrawn. - **`CheckpointCoordinator` rename (F6, F8):** parameter rename only (L224, used at L285); the value flows from `JobMaster.initCheckPointManager` as `jobImmutableInformation.isRestoreJob() || restart` (L343), which already carried the "any restore" meaning. Withdrawn. - **`JobException` on checkpoint restore (F4):** the pending-cleanup and existing-metrics checks (L1429-L1465) still use the caller-supplied flag, so this is not a behavior change. Withdrawn. **Remaining ask** - **`JobMaster.handleSaveMode` (F3):** agreed this matches `dev` — `JobMaster.init` (L276-L293) only calls it for non-restore jobs, and the comment at L275-L276 records that. The restore branch (`handleSchemaSaveModeWithRestore`) is only reachable through the deprecated boolean overload and external callers. Please add one Javadoc sentence on `handleSaveMode(SeaTunnelSink, RestoreMode)` (L788) stating that, so the next reader doesn't assume it's dead code. Downgrading this to a doc nit; happy to approve once it's in. <!-- streview-comment:2262 --> -- 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]
