DanielLeens commented on PR #11841:
URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-5739292902
Hi @JeremyXin, thank you for laying the two options out so clearly, and
thanks @SEZ9 for the recommendation. I re-traced this against `dev` and against
the released `v3.0.0` and `2.3.13` tags (not only against the earlier revision
of this PR), and I owe you a correction.
**Decision: keep Option 1 exactly as it is at `e0b46feedf`. Please do not
restore the payload re-derivation, and please do not attempt Option 2 in this
PR.** I am withdrawing Blockers #1 and #2 from my last review. I will update my
review state formally when the next commit lands.
**Why I am withdrawing them.** I measured the change against the earlier
revision `7f55adf8` instead of against what actually ships. The facts on the
pinned head and the tags:
- `CoordinatorService.submitJob` on `dev` and on `v3.0.0` already consumed
the caller-supplied boolean (`v3.0.0:CoordinatorService.java:1410/1419/1443`,
same three spots as `CoordinatorService.java:1412/1421/1445` on your head). In
`2.3.13` the same parameter is the only signal too
(`CoordinatorService.java:652`). The net diff of this PR in that file is only
the two comment lines at `:1389-1390`.
- Every released producer already sends the savepoint-only value. `v3.0.0`
`ClientJobProxy.java:77` and `BaseService.java:1363/1397` pass
`isStartWithSavePoint()`, and the constructor sets that field to `restoreMode
== RestoreMode.SAVEPOINT` (`v3.0.0:JobImmutableInformation.java:119`, unchanged
on your head at `:119`). Your switch to `isSavepointRestore()`
(`ClientJobProxy.java:77`, `BaseService.java:1363`, `:1397`) is therefore a
value-preserving rename. My earlier worry that an older node computes this
boolean with different logic does not hold: no released version does.
**Rolling upgrade, both directions (traced from source, not run in a mixed
cluster):**
| Producer -> master | What happens |
|---|---|
| 2.3.x client or node -> new master | Wire boolean is the savepoint flag.
The payload has no trailer, so `JobImmutableInformation.readData` derives
`SAVEPOINT`/`NONE` and `restoreSourceJobId = jobId` from the legacy field
(`:259-260`) before checking for a trailer (`:261`). Both carriers agree. |
| v3.0.0 client or node -> new master | Same values as a new client. |
| new client or node -> v3.0.0 master | Same booleans, same coordinator
logic. A `CHECKPOINT` restore arrives as `false` on both. |
| new client -> 2.3.x master | `SAVEPOINT` works. A `CHECKPOINT` restore
cannot be honored by a master that has no `RestoreMode`. This is inherent to
the v3.0.0 feature and no choice of wire boolean fixes it, since `true` would
make the old master restore from the wrong job id. My reading is that the old
`readData` simply ignores the trailer, so this is worth one line in the docs
("checkpoint restore needs 3.0.0 or later on the master"), not a code change
here. |
**Option 2** is the wrong fit here: `SeaTunnelSubmitJobCodec.java:67-97` and
`SubmitJobOperation.java:54/61` carry a fixed-layout field, so dropping it
breaks a new client talking to an older master. That needs its own deprecation
plan, as we discussed on 2026-09-03.
**On @SEZ9's proposal**, point by point:
1. Keep the parameter for wire compatibility: agree. One wording caution:
please do not mark it "no longer consulted on the master side" while
`submitJob` still consults it at `:1412/:1421/:1445`. The note has to match the
code.
2. "Fall back to the legacy field when `restoreMode` is null": this already
exists. `readData` sets the mode from the legacy boolean at `:259` before
reading the trailer, the constructor coerces null to `NONE` (`:117`), and
`RestoreMode.fromCode` throws on unknown codes, so a deserialized payload never
carries a null mode. Adding another fallback would be dead code. The legacy
shape is already pinned by `shouldReadLegacyPayload` and
`shouldReadLegacyPayloadWithUnsafeInput` in
`JobImmutableInformationCompatibilityTest`.
3. Derive the coordinator decision from the payload: this would also be safe
in all four rows above, given (2). But it is the `7f55adf8` design and changes
the shipped coordinator semantics, so I would not require it in this PR. It is
a fine follow-up if you want a single authority later.
4. `ClientJobProxy` sends `true` only for savepoint: already true at
`ClientJobProxy.java:77`.
**Concrete changes for this PR (small):**
1. `CoordinatorService.java:1389-1390`: reword the comment to say the master
honors the caller-supplied flag, that every producer (2.3.x, 3.0.0 and the
three current call sites) sends the savepoint-only value, and that `CHECKPOINT`
restores therefore arrive as `false`.
2. `CoordinatorServiceJobCleanupTest`: rename
`testSubmitSavepointUsesLegacyParameter` (`:317`) to something like
`testSubmitHonorsCallerSuppliedSavepointFlag`, and add a sibling that submits a
`RestoreMode.CHECKPOINT` payload (source job `destinationJobId - 1`, absent
from the running set so `CheckpointRestoreValidator` passes) with wire `false`
and a pending cleanup record. Assert the `JobException` containing "waiting for
terminal state cleanup" and that the record is retained. This pins the
checkpoint-mode behavior @SEZ9 asked to document and gives the path the
coverage it lacked.
3. `JobImmutableInformationCompatibilityTest`: add a small test that for
each `RestoreMode` value `isSavepointRestore() == isStartWithSavePoint()`,
including after a `writeData`/`readData` round trip, plus the negative legacy
case (legacy `false` gives `NONE` and `isRestoreJob() == false`). This is the
invariant that makes the caller-supplied flag safe.
4. PR description: note that `CHECKPOINT` restores which hit a pending
cleanup record now take the `JobException` path. This matches `v3.0.0`, so
please describe it as documented behavior rather than a new change.
**The two remaining points from earlier**, briefly:
`CheckpointCoordinator`'s `isRestoreJob` is already fed by `JobMaster.java:342`
(`isRestoreJob() || restart`), so nothing to add. For `handleSaveMode`, the
guard at `JobMaster.java:277` only replaces
`logicalDag.isStartWithSavePoint()`, which on the client path already carried
"any restore" semantics, so restore jobs skipped that block before this PR too.
Moving the call outside the guard would start running cluster-side schema
handling for restore jobs, which is a behavior change and better as its own
issue and PR. In this PR I would leave it as is.
Thanks again for the patience through the back and forth. The design you
converged on is the right one, and the fix on my side was to compare it against
the right baseline.
--
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]
