SEZ9 commented on PR #11841:
URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6281811057
Thanks @JeremyXin for rechecking against c1c542de8cc8. Going through your
points in order:
1. **Legacy payload / null `restoreMode`.** If `readData()` seeds
`restoreMode` from the legacy boolean before the optional trailer is read, that
addresses my concern for the legacy `isStartWithSavePoint=true` case. Could you
confirm `shouldReadLegacyPayload` asserts both `isRestoreJob()` and
`isSavepointRestore()` (not just the enum value), so the
`CoordinatorService.submitJob` path is covered end-to-end? Relatedly, if
`restoreMode` can never be null after `readData()`, please either drop the null
guard on the boolean derivation in `submitJob` or guard the following
dereference too; right now one line implies it can be null and the next assumes
it can't.
2. **`ClientJobProxy` wire boolean.** Understood that `isSavepointRestore()`
is intended to be true only for SAVEPOINT, which would keep the wire value
identical to the old flag. A small test asserting the value sent for each
`RestoreMode` would make that easy to confirm.
3. **Unreachable restore branch in `handleSaveMode`.** Thanks for
confirming; I'm fine deferring the cluster-side behavior decision to a
follow-up. For this PR, please either remove the dead `RestoreMode` branch or
leave a short TODO pointing at the follow-up so a future reader doesn't assume
restore jobs get save-mode handling there.
4. **Pending-cleanup / existing-metrics branches.** Understood that the diff
only adds a comment there. My concern is not the branches themselves but what
now reaches them: does a checkpoint-mode restore now arrive with the savepoint
boolean unset and therefore take the `JobException` path where it previously
took the savepoint path? If so, that is the user-facing change I'd like
documented; if not, please point me to where it is handled. Filling in the
user-facing-change and testing sections of the description as you mentioned
would help here.
5. **Remaining points.** Your comment appears to be cut off after point 4
("including the existing chec…"), so I didn't see the rest of your list. Could
you repost that part? In particular, the `CheckpointCoordinator` rename is
still open from my side: which constructor call site now passes
`isRestoreJob()` rather than the savepoint-only flag, and is there a test
exercising checkpoint-mode restore through it?
<!-- streview-comment:1882 -->
--
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]
