DanielLeens commented on code in PR #11841:
URL: https://github.com/apache/seatunnel/pull/11841#discussion_r3923497074
##########
seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:
##########
@@ -1394,9 +1396,12 @@ public PassiveCompletableFuture<Void> submitJob(
try {
JobImmutableInformation
submittedJobImmutableInformation =
deserializeJobImmutableInformation(jobImmutableInformation);
+ boolean isSavepointRestore =
Review Comment:
Good question, and worth thinking through carefully since it's a wire-format
decision, not just an internal refactor.
My take: **Option 1** for now, not Option 2 — with a caveat.
The two are not equivalent in blast radius. `submitJob`'s
`isStartWithSavePoint` boolean is also carried on `SubmitJobOperation`'s
serialized wire format (`writeData`/`readData`), which is what makes
mixed-version rolling upgrades safe — an old client/master still emits and
reads that raw boolean. Option 2 (remove `isStartWithSavePoint` from
`submitJob(...)` and clean up the operation/codec path) touches that wire
contract directly, so it can't land as a single PR without breaking a cluster
mid-rolling-upgrade — it would need a proper deprecate-then-remove cycle
spanning at least one release, with the codec accepting both shapes in between.
Option 1 (always derive from `JobImmutableInformation`, drop only the
*internal* re-derivation in `submitJob(...)`) gets you the same practical
outcome — one source of truth for the restore-mode decision inside
`CoordinatorService` — without touching the RPC/REST wire format at all, since
`JobImmutableInformation` is already serialized as part of the same payload and
its own `readData()` already handles the pre-`RestoreMode` fallback. That's the
lower-risk path and I'd scope the follow-up to that.
If Option 2 is still desired long-term (fully retiring the legacy boolean
from the wire), I'd treat that as a separate, explicitly-flagged compatibility
change with its own deprecation window — not something to fold into a "quick
cleanup" follow-up.
##########
seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/master/JobMaster.java:
##########
@@ -284,7 +285,7 @@ public synchronized void init(long initializationTimestamp,
boolean restart) thr
sink.getId()));
JobMaster.handleSaveMode(
((SinkAction<?, ?, ?, ?>)
sink).getSink(),
- logicalDag.isStartWithSavePoint());
Review Comment:
Makes sense, thanks for spelling that out — that's consistent with what I
traced too (`LogicalDagGenerator` already receives `restoreMode.isRestore()`,
so `LogicalDag.isStartWithSavePoint()`'s underlying value hasn't changed, just
where the master reads the restore decision from). No concern here; the
naming-smell follow-up I mentioned above is a nice-to-have, not something
blocking this PR.
--
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]
