DanielLeens commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6616918795
Thanks @SEZ9, these are fair questions about my review. I re-read them against head `c1cb6137` (static read only; the PR's own hunks are unchanged since `881e275f`): 1. **Legacy payloads:** `JobImmutableInformation.readData` seeds `restoreMode` from the legacy flag first (`JobImmutableInformation.java:261`, savepoint flag -> `SAVEPOINT`, otherwise `NONE`) and only overrides it when the new trailer is present (L263). `JobImmutableInformationCompatibilityTest` covers both legacy shapes (with and without the savepoint flag). `CoordinatorService.submitJob` does not read `restoreMode` at all; it still branches on the caller-supplied boolean. 2. **Null handling in `submitJob`:** there is no `restoreMode` in `CoordinatorService.submitJob(long, Data, boolean)`; the PR adds only a 4-line comment there (L1404-L1406). The normalization you suggest already happens once, in the `JobImmutableInformation` constructor (`restoreMode == null ? RestoreMode.NONE : restoreMode`, L117), and `isRestoreJob()` (L167) is null-safe on top of that. 3. **Mixed-version wire boolean:** `isSavepointRestore()` (L172) is true only for `SAVEPOINT`, which is exactly what the old `isStartWithSavePoint()` returned. A `CHECKPOINT` restore from a new client sends `false`, the same as on `dev`, so an old master sees the same bytes and takes the same branch. The `writeData`/`readData` and codec layouts are untouched. 4. **`JobMaster.handleSaveMode`:** correct, `JobMaster.init` (L276-L293) only calls it for non-restore jobs, and that matches `dev`: restore jobs already skipped cluster-side save mode via `!logicalDag.isStartWithSavePoint()`, and the new comment at L274-L275 records that. The restore branch (`handleSchemaSaveModeWithRestore`) is preserved public-helper behavior, reached through the deprecated boolean overload and external callers. This is my Issue 3: one Javadoc sentence on `handleSaveMode(SeaTunnelSink, RestoreMode)` (L788) would make that explicit. 5. **`CheckpointCoordinator` rename:** parameter rename only (L224, used at L285). The value comes from `JobMaster.initCheckPointManager`, which passes `jobImmutableInformation.isRestoreJob() || restart` (L343) into `CheckpointManager`; that expression already had the "any restore" meaning before the PR. 6. **`JobException` on checkpoint restore:** not a behavior change. The pending-cleanup and existing-metrics checks (`CoordinatorService.java` L1429-L1465) still use the caller-supplied flag, so a checkpoint restore fails there exactly as on `dev`; the PR description now states this. So my reading is still a value-preserving refactor. If you find a producer or payload where one of these expressions differs from `dev`, please point me at it and I will re-check. -- 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]
