SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6331077191
@JeremyXin Thanks for rechecking against e7c81d5dd and for the corrections — that clears up several of my misunderstandings. 1. **Legacy payload classification (F1):** Having `shouldReadLegacyPayload()` assert both `isRestoreJob()` and `isSavepointRestore()` when the legacy savepoint flag is set is exactly the coverage I was after. I'll verify it against the 9fa4066ee changes before marking this one closed. 2. **Wire boolean / submitJob (F2, F5, F7):** Appreciate the correction. If `CoordinatorService.submitJob` still honors the caller-supplied boolean for the cleanup and job-metrics checks, and the client/REST paths send `isSavepointRestore()` with the same true-only-for-savepoint meaning as before, then F2 and F5 are narrower than I wrote. I'll confirm `restoreModeShouldMatchLegacySavepointFlag()` and `testSubmitHonorsCallerSuppliedSavepointFlag` on the diff. One follow-up on F7: is the unconditional `restoreMode` dereference right after the null-guarded derivation still present, or has that path been simplified? If it's still there, a null-safe read (or a comment explaining why null is impossible at that point) would close it. 3. **Cluster-side save-mode handling (F3):** Your message appears to have been cut off at "The current comment" — could you repost the rest? Specifically, is the restore-aware branch of `handleSaveMode(SeaTunnelSink, RestoreMode)` now reachable from its call site, or is the intent that CLUSTER-side restore jobs intentionally skip schema save-mode handling? If the latter, a comment at the call site saying so would be enough. Two remaining asks: - **F4:** A short note in the PR description that checkpoint-mode restores which hit a pending cleanup record or existing job metrics now fail with `JobException` instead of following the savepoint path. The behavior sounds intended; I just want it documented for people upgrading. - **F6 / F8:** Could you point me to where the `CheckpointCoordinator` constructor call site(s) were updated to pass `isRestoreJob` rather than the savepoint-only flag? If that landed in 9fa4066ee or e7c81d5dd I may have missed it; if not, that's the last functional gap I'd want closed before approving. Once I have the rest of point 3 and the pointer for F6/F8, I'm happy to take another pass. <!-- streview-comment:1930 --> -- 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]
