DanielLeens commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-5726089173
Thanks both — went back through `e0b46feedf` (unchanged since my last review) to check the callers and each of the five open items. **On the caller-derivation question, @JeremyXin:** confirmed, all three in-repo submission call sites populate the wire boolean from `isSavepointRestore()`, not the reverted `submitJob`-internal re-derivation: - `ClientJobProxy.java:77` - `BaseService.java:1360-1363` (cross-node dispatch) - `BaseService.java:1394-1397` (direct submission) That said, I don't think this resolves the concern behind Blocker #1/#2 in my review, and your own comment actually reinforces why: you noted the legacy `isStartWithSavePoint` wire field is kept specifically "for rolling-upgrade compatibility." That's exactly the scenario my review was pointing at — a pre-this-PR node in a mixed-version cluster computes that wire boolean with its own (pre-unification) logic, not `isSavepointRestore()`, since that method didn't exist on its side yet. Once `CoordinatorService.submitJob` stopped re-deriving from the deserialized `JobImmutableInformation` and started trusting the parameter directly (`CoordinatorService.java:1412`, `:1421`, `:1445`), the coordinator has no way to catch a mismatch from that kind of producer during a rolling upgrade. I'm not saying today's three callers are wrong — they aren't — I'm saying the safety net that made the coordinator's decision correct *regardless* of the producer is gone, and a rolling upgrade is prec isely the window where that matters. I'd still like to see this either restored inside `submitJob` or split into the follow-up PR we discussed on 2026-09-03, with a rolling-upgrade note. Happy to be shown a reason mixed-version submission can't actually reach this path if I'm missing something. Now the five items, @SEZ9: **1. Legacy payloads with `restoreMode == null`.** This case can't actually occur — `restoreMode` is never left null. In `JobImmutableInformation.readData` (`JobImmutableInformation.java:259-268`), the field is unconditionally set from the legacy boolean first (`restoreMode = isStartWithSavePoint ? RestoreMode.SAVEPOINT : RestoreMode.NONE`), then only overwritten if a new-format trailer is present. So an old-format payload with `isStartWithSavePoint=true` deterministically becomes `RestoreMode.SAVEPOINT`, never `null`. Same on construction: the multi-arg constructor normalizes `restoreMode == null ? RestoreMode.NONE : restoreMode` (line 117). So `isRestoreJob()`/`getRestoreMode()` are safe to call unconditionally anywhere downstream. However, digging into "how is it classified downstream" turned up a real, separate bug — see #3 below, which I think is the actual data-loss risk you were pointing at. **2. Null handling in `submitJob`.** Confirmed clean. The current `submitJob` body (`CoordinatorService.java:1408-1452`) never reads `restoreMode` at all — it only reads the `isStartWithSavePoint` parameter and calls `submittedJobImmutableInformation` through `validateCheckpointRestoreSourceJobIsTerminal` -> `CheckpointRestoreValidator.validate` (`CheckpointRestoreValidator.java:35-38`), which null-checks `jobImmutableInformation` itself before ever touching `.getRestoreMode()`. No null-guard-then-unconditional-dereference pattern anywhere in this path. **3. Save-mode handling in `JobMaster` — confirmed, this is real.** You're right, and it's worse than "unreachable branch": restore jobs get zero cluster-side schema save-mode handling at all here, not just a code path that happens to always take the non-restore branch. - `JobMaster.init` only calls `handleSaveMode` inside `if (!restart && !jobImmutableInformation.isRestoreJob() && ...)` (`JobMaster.java:276-293`). - Since that guard already requires `isRestoreJob() == false`, the value passed at line 292 (`jobImmutableInformation.getRestoreMode()`) can only ever be `RestoreMode.NONE` at that call site. - Inside the static `handleSaveMode(sink, restoreMode)` (`JobMaster.java:780-792`), `isRestoreJob = restoreMode != null && restoreMode.isRestore()` (line 781) is therefore always `false` when reached from `init`, so the `handler.handleSchemaSaveModeWithRestore()` branch (line 791) is dead code via this caller, and actual restore jobs (`isRestoreJob() == true`) never call `handleSaveMode` at all — they skip the entire block at line 276. That does mean cluster-side schema save-mode handling is silently skipped for every restore job, which sounds like the `SaveMode` data-loss risk you flagged. This looks pre-existing to the `RestoreMode` unification (the `!isRestoreJob()` guard governs whether the block runs at all, and nothing in this PR's diff touches that specific gate), but it's a legitimate independent bug in the current head regardless of blame — worth its own fix (and its own test) either in this PR or a fast-follow, since it sits on the checkpoint/savepoint restore path same as my Blocker #1. **4. `CheckpointCoordinator` `isRestoreJob` parameter — already wired correctly, I think this one's resolved.** The constructor call site is updated, just one hop further out than where you were looking: `JobMaster.initCheckPointManager` passes `jobImmutableInformation.isRestoreJob() || restart` (the "any restore" flag) into `new CheckpointManager(...)` at `JobMaster.java:342`. `CheckpointManager`'s own constructor parameter is named `isRestoreJob` (`CheckpointManager.java:98`) and is forwarded unchanged into `new CheckpointCoordinator(..., isRestoreJob, ...)` at `CheckpointManager.java:149-160`, landing on `CheckpointCoordinator`'s `isRestoreJob` parameter (`CheckpointCoordinator.java:222`). So checkpoint restores do pass the restore flag, not the savepoint-only one — let me know if you were tracing a different construction path. **5. Behavior change documentation.** Confirmed accurate as a description of the current head: since the wire `isStartWithSavePoint` now equals `isSavepointRestore()` (savepoint-only) for all three callers, a `CHECKPOINT`-mode restore that hits a pending cleanup record or pre-existing job metrics takes the `else` branches at `CoordinatorService.java:1423-1428` and `:1445-1452` and fails with `JobException` rather than going through the savepoint cleanup path. Agreed this needs a line in the PR description/release notes regardless of how #1 in my review gets resolved, since it's a real, user-visible behavior change on the checkpoint-restore path. -- 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]
