JeremyXin commented on PR #11421:
URL: https://github.com/apache/seatunnel/pull/11421#issuecomment-5103889475
> Thanks for working on this. I re-reviewed the latest head `7eac1b316989`
from scratch.
>
> # What this PR fixes
> * User pain: A canceled/failed Zeta streaming job could not be resumed
from its last successful checkpoint — checkpoint data was cleaned up on cancel
and there was no restore entrypoint, forcing users to re-read all source data
(#10926).
> * Fix approach: Adds a `CHECKPOINT` restore mode wired end-to-end — a new
`--restore-with-checkpoint <job-id>` CLI flag and
`restoreMode=CHECKPOINT&restoreSourceJobId=` REST parameter, a
`checkpoint.retain-after-job-cancelled` option at both job-`env` and cluster
(`seatunnel.yaml`) level (job-level wins), fail-fast validation when checkpoint
data is missing/incompatible, and a new client execution context
(`restoreFromCheckpointExecutionContext`) that hands the source job id through
submission, checkpoint lookup, and runtime recovery.
> * One-line summary: The feature is coherent and well-documented (EN+ZH
docs, CLI arg validation with mutual-exclusion checks and numeric job-id
normalization, plus new unit tests), but I want to flag the signature/protocol
change in `GetJobCheckpointTask` as a potential compatibility break for rolling
upgrades and mixed-version clusters before this merges.
>
> # Runtime chain I rechecked
> ```
> CLI restore-with-checkpoint path (rechecked):
> ClientCommandArgs.buildCommand() ClientCommandArgs.java:155
> -> mutual-exclusion checks (--restore vs --restore-with-checkpoint,
--savepoint vs --restore-with-checkpoint) ClientCommandArgs.java:155-162
> -> normalizeNumericJobId(restoreWithCheckpointJobId)
ClientCommandArgs.java:190
> -> new ClientExecuteCommand(this) ClientCommandArgs.java:187
> ClientExecuteCommand.execute() ClientExecuteCommand.java:191
> -> engineClient.restoreFromCheckpointExecutionContext(configFile,
variables, jobConfig, seaTunnelConfig, restoreSourceJobId, customJobId)
ClientExecuteCommand.java:194-205
> -> job submission with RestoreMode.CHECKPOINT + restoreSourceJobId
propagated to the coordinator
> -> GetJobCheckpointTask (member protocol task) resolves latest
completed checkpoint for restoreSourceJobId GetJobCheckpointTask.java
(signature changed in this PR — flagged as public API/protocol compatibility
risk)
> -> CheckpointManager loads checkpoint state into the new run
(new runtime jobId) instead of a fresh start
>
> Retention path (rechecked):
> EnvCommonOptions.CHECKPOINT_RETAIN_AFTER_JOB_CANCELLED (no default)
EnvCommonOptions.java:104-110
> -> registered in EnvOptionRule.optionRule() EnvOptionRule.java:46
> -> job-level env value, when present, overrides cluster default
seatunnel.engine.checkpoint.retain-after-job-cancelled at cancel-time cleanup
> ```
>
> # Findings
> **Issue 1: Possible public API change in
seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/protocol/task/GetJobCheckpointTask.java**
>
> * Location:
`seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/protocol/task/GetJobCheckpointTask.java`
> * Why it matters: A public/protected signature appears to be removed or
changed. Verify backward compatibility for downstream connectors.
> * Severity: High
>
> # Review conclusion
> ### Conclusion: can merge after the blocking items are fixed
> **1. Blocking items**
>
> * Issue 1: Possible public API change in
seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/protocol/task/GetJobCheckpointTask.java
>
> Nice progress overall — once the blocking points above are addressed this
should be in good shape. Happy to discuss.
Thanks for the careful re-review. I think the compatibility risk here is
more limited than a public API break in
GetJobCheckpointTask itself. The current implementation keeps the legacy
encodeRequest(long jobId) path unchanged, and
SeaTunnelGetJobCheckpointCodec remains able to decode legacy requests by
mapping them to the original SAVEPOINT behavior, which
preserves backward compatibility for the common existing flow.
GetJobCheckpointTask is also an internal server-side protocol
task, not a downstream public API surface. That said, if there are
particular mixed-version or rolling-upgrade scenarios that
need stricter guarantees, I agree those can be further hardened in
follow-up iterations.
--
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]
