SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-5944639870
Thanks for the follow-up in `af98c93c064`. **Test invariant** — closed. Building `JobImmutableInformation` through the real constructor and asserting `isSavepointRestore()` / `isRestoreJob()` alongside `isStartWithSavePoint()` for every `RestoreMode` value, before and after the `writeData`/`readData` round trip, covers the gap in the earlier reflection-based version. The new `shouldReadLegacyPayloadWithoutSavepoint` test (`isStartWithSavePoint=false` -> `RestoreMode.NONE`) is a nice addition. One related question: that test covers the negative legacy shape. Is the positive legacy shape — `restoreMode == null` with `isStartWithSavePoint == true` — also asserted to still be classified as a savepoint restore? If not, please add that case so the legacy-client restore path is pinned down too. **PR description** — the reworded "What does this PR do / Why is this needed" section now matches the intended framing. Two sections are still blank: 1. **"Does this PR introduce any user-facing change?"** — please answer explicitly. In particular, call out that checkpoint-mode restore submissions hitting a pending cleanup record or existing job metrics now fail with `JobException` instead of following the savepoint path, and clarify whether there are any expectations for mixed-version clusters (e.g. an older master receiving a submission from a newer client). 2. **"How was this patch tested?"** — list the compatibility tests above and anything else you ran (e.g. a manual savepoint/checkpoint restore round-trip) so reviewers and release notes can rely on it. Once those two sections are filled in and the positive legacy-shape case is either pointed to or added, I think this is ready to move forward. <!-- streview-comment:1458 --> -- 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]
