SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6306456876
@JeremyXin thanks for the detailed follow-up on 9d446cb99. Going through your points against my earlier findings: 1. **Legacy payload classification** — `shouldReadLegacyPayload()` asserting both `isRestoreJob()` and `isSavepointRestore()` for a legacy payload with the savepoint flag set is the coverage I was looking for. I'll confirm against the diff at 9d446cb99 before closing this out. 2. **Legacy flag vs. RestoreMode** — `restoreModeShouldMatchLegacySavepointFlag()` covers the flag values, but my concern was also about `submitJob` deriving savepoint-restore only from `RestoreMode` (ignoring the legacy `isStartWithSavePoint` field) and about `ClientJobProxy` changing the meaning of the wire boolean for old masters. Could you point me to where those two code paths changed in 9d446cb99, or clarify if the test alone is meant to address them? 3. **Restore jobs skipping cluster-side save-mode handling** — a clarifying comment at the `JobMaster.init` guard is fine for now, with a follow-up issue linked in the PR description so this doesn't get lost. One thing to clear up: my finding was that the restore-aware branch of the new `handleSaveMode` is unreachable because of that guard, so CLUSTER-side restore jobs lose save-mode handling as a result of this PR. If you believe this behavior predates the PR, could you point me to where it was already skipped? 4. **Pending cleanup / existing metrics on checkpoint restore** — you mention these checks are unchanged, but my finding was that checkpoint-mode restores previously followed the savepoint path here and now hit `JobException`. Could you walk me through why you see no behavior change? If there is one, documenting it in the PR description as an upgrade note would be enough for me. 5. **CheckpointCoordinator flag** — `initCheckPointManager()` passing `isRestoreJob() || restart` through `CheckpointManager` to `CheckpointCoordinator` is the right shape. I'll verify that call site is part of the diff at 9d446cb99, since my earlier read didn't find a modified call site. 6. **Null handling in `submitJob`** — your comment appears cut off at "readData() initializes resto…". Could you finish that thought? If `readData()` guarantees `restoreMode` is non-null for legacy payloads, that would address it. So the remaining asks are: pointers for (2), clarification on (3) and (4), and the completed explanation for (6). I'll re-check (1) and (5) against the diff. Thanks again for the thorough work here. <!-- streview-comment:1907 --> -- 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]
