DanielLeens commented on code in PR #11841:
URL: https://github.com/apache/seatunnel/pull/11841#discussion_r3922135226
##########
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:
That's a completely fair challenge, and I want to answer it precisely rather
than just restate the earlier "follow-up" framing — because I think the honest
answer is that this PR is unifying one specific thing, and the wire argument is
a different thing that happens to share a name.
**What this PR actually unifies:** the *decision logic* that turns "is this
a restore, and which kind" into behavior — `LogicalDag`'s stale boolean,
`JobMaster.init`/`handleSaveMode`'s pre-collapsed flag,
`MultipleTableJobConfigParser`, and `CoordinatorService.submitJob`'s
cleanup-ownership branch (line 1399 here) all now derive from the single
`JobImmutableInformation.getRestoreMode()` source of truth instead of five
independent booleans that could (and, per the bug this PR actually fixes, did)
drift out of sync. That's the "Unified Processing" in the title, and it's fully
done in this diff —
`CoordinatorServiceJobCleanupTest.testSubmitCheckpointRestoreDoesNotTrustLegacySavepointFlag`
is specifically there to lock in that the coordinator's decision no longer
trusts the legacy flag when it disagrees with the real `RestoreMode`.
**What's deliberately *not* touched here:** the `isStartWithSavePoint`
boolean that still exists on `SubmitJobOperation.writeInternal`/`readInternal`
and on `SeaTunnelSubmitJobCodec`
(`REQUEST_IS_START_WITH_SAVE_POINT_FIELD_OFFSET`). I want to be concrete about
why this is a genuinely different category of change, not just a convenient
place to stop early:
- `SeaTunnelSubmitJobCodec` is a `@Generated` Hazelcast **client protocol**
codec — the header literally says it's generated from
`client-protocol-definition/SeaTunnelEngine.yaml` against the
hazelcast-client-protocol spec. This is the binary format the SeaTunnel CLI
client speaks to the Zeta master over the network.
- `SubmitJobOperation` is a Hazelcast cluster `Operation`, `Data`-serialized
and keyed by `ClassId`
(`ClientToServerOperationDataSerializerHook.SUBMIT_OPERATOR`) — the format
cluster members/clients exchange for this RPC.
Removing a field from either of those isn't a Java-level rename I can do
inside this refactor; it's a wire-format change that has to survive **version
skew**: an old CLI submitting to a freshly-upgraded master, or a master/worker
mix mid rolling-restart, would decode a truncated frame and fail hard if the
field just disappears. That needs its own plan — a deprecation window, a codec
version bump, or a documented "removed in vX, minimum client version Y" note in
the compatibility docs — which is exactly the kind of thing that shouldn't get
bundled into what is otherwise a clean, additive, single-purpose logic refactor
with tests already anchored to today's behavior. Mixing the two would also make
this PR harder to review and revert independently if either half needed to be
backed out.
To be clear, I'm not disagreeing that the wire-level retirement should
happen — @SEZ9 and I already discussed this in the top-level thread on
2026-08-20/21 and agreed it's real follow-up work, not a "maybe." What I pushed
back on then (and still would) is *me* filing that tracking issue as part of a
review reply, since scoping "when every reader has migrated off the boolean" is
a call for @JeremyXin/a maintainer to make, not something I should decide
unilaterally in a comment thread.
So: if you'd like to see this land as its own next step, opening that
tracking issue now (referencing this thread) is the right move — I'm glad to
review it. But I'd keep it out of this PR; this PR's job was to stop the
decision logic from trusting the stale boolean, and it does that with
regression coverage to prove it. No new blocker from me here — my 2026-08-17
approval stands.
##########
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:
Thanks for confirming — appreciate you pushing on it, it's a legitimate
naming smell worth having on record even though it predates this PR. If you'd
like to file the `LogicalDag.isStartWithSavePoint` → `isRestoreJob` rename as
its own small follow-up (Javadoc-first, keep the wire-serialized accessor name
for compatibility, same treatment this PR already used on
`JobImmutableInformation.isStartWithSavePoint()`), I'm happy to review it. Not
something this PR needs to carry.
--
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]
