DanielLeens commented on PR #11841:
URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-5370780625
Answering both asks:
1. **Tracking issue for deprecating the legacy `isStartWithSavePoint` wire
argument**: I'd rather not file that myself here — opening and scoping a
follow-up issue is better owned by @JeremyXin or a maintainer at merge time,
since it should also decide *when* it's safe (once every reader has migrated
off the boolean, per the rolling-upgrade note in my Compatibility Impact
section). Happy to comment on it once it exists, but I don't want to create
GitHub issues on your behalf as part of a review reply.
2. Re-checked this directly against the diff rather than from memory: the
newly added `testSubmitCheckpointRestoreDoesNotTrustLegacySavepointFlag`
submits a `RestoreMode.CHECKPOINT` payload with the legacy wire boolean set to
`true`, and asserts it still throws `JobException("...waiting for terminal
state cleanup...")` with the pending-cleanup record left intact — i.e. it
proves the **checkpoint**-restore path does *not* get routed through
savepoint's cleanup-ownership branch (`isSavepointRestore == true`) even when
the stale wire flag disagrees. It does not itself exercise the
`isSavepointRestore == true` branch.
That branch (`cleanupPendingPipelineCleanupForRestore` /
`cleanupPendingJobStateForRestore` actually running) continues to be covered by
two pre-existing, unmodified tests in the same file:
`testSubmitStartWithSavePointAllowedWhenCleanupStillPending` and
`testSubmitStartWithSavePointClearsStaleTerminalState`. I traced both against
`createJobData(jobId, true)` -> the legacy `JobImmutableInformation(...,
boolean isStartWithSavePoint, ...)` constructor, which sets `restoreMode =
RestoreMode.SAVEPOINT` when `true` -> `isSavepointRestore() == true`, and both
assert `assertDoesNotThrow`, i.e. the savepoint cleanup path runs to completion.
So between the one new test and the two pre-existing ones, both branches
of `CoordinatorService.submitJob`'s `isSavepointRestore` check are exercised
end to end — just not by a single test. No gap here from my side.
My 2026-08-17 approval stands either way.
--
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]
