SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-5366259821
Thanks @DanielLeens for the thorough review and the follow-up answers. On point 1, I agree with your conclusion: keeping the legacy `isStartWithSavePoint` wire argument untouched in this PR is the right call. Since `SubmitJobOperation`'s wire format is unchanged and the field and the derived `RestoreMode` agree for every currently-reachable payload, deprecation isn't forced here, and mixed old/new deploys need the field on the wire anyway. Deferring that to a follow-up once all readers have migrated to the `RestoreMode`-derived accessors keeps this PR a clean, additive refactor. On point 2, thanks for re-pulling the review body from the API to confirm it's complete — good to know the apparent truncation was just a rendering hiccup on the `CoordinatorService.java` before/after block and nothing was missing from section 1.1. Two concrete asks before we wrap this up: 1. Could you open (or link, if one exists) a tracking follow-up for deprecating the legacy `isStartWithSavePoint` wire argument once every reader has moved to the `isSavepointRestore()` / `isCheckpointRestore()` / `isRestoreJob()` accessors? I'd like that on record so it doesn't get lost after merge. 2. Please confirm the added coordinator-level unit test (the scenario where the legacy wire boolean disagrees with the payload's real restore mode) exercises the savepoint-only cleanup paths in `CoordinatorService.submitJob` specifically — that's the behaviorally interesting hunk you called out, and I want the regression coverage anchored there. With your 2026-08-17 approval standing and no new blockers raised, this looks ready to move toward a merge decision once those two items are confirmed. <!-- streview-comment:437 --> -- 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]
