JeremyXin commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6306726860
@SEZ9 Thanks for following up. I rechecked the current head, e7c81d5dd, and want to clarify a few points where my earlier replies were imprecise: 1. Legacy payload classification: shouldReadLegacyPayload() now asserts both isRestoreJob() and isSavepointRestore() when the legacy savepoint flag is true. This was added in commit 9fa4066ee. 2. Wire boolean and submitJob: ClientJobProxy and both REST submission paths pass jobImmutableInformation.isSavepointRestore() as the legacy boolean. restoreModeShouldMatchLegacySavepointFlag() verifies that this value matches isStartWithSavePoint() for every RestoreMode, including after serialization round trips. It does not capture the outgoing request at the call site. One correction to my earlier wording: CoordinatorService.submitJob still uses the caller-supplied boolean for its cleanup and job-metrics checks; it does not re-derive that decision from the payload’s RestoreMode. The test testSubmitHonorsCallerSuppliedSavepointFlag covers that behavior. The wire field’s meaning has not changed: it remains true only for a savepoint restore, as it was for constructed submissions before this PR. 3. Cluster-side save-mode handling: The current comment at the JobMaster.init guard documents that restore jobs skip this handling. This predates the PR: on dev, the same guard used !logicalDag.isStartWithSavePoint(), and that DAG flag was populated from restoreMode.isRestore(). The PR replaces that predicate with the equivalent jobImmutableInformation.isRestoreJob(). I agree the broader behavior deserves a follow-up; there is no tracking issue linked yet, so I will track it separately rather than imply that this PR changes the behavior. 4. Checkpoint restore cleanup and metrics: These coordinator branches are unchanged from dev. The submission paths passed isStartWithSavePoint() before this PR and now pass isSavepointRestore(); both are true only for SAVEPOINT. Therefore a checkpoint restore passes false and follows the existing JobException path when cleanup is pending or job metrics already exist. testSubmitCheckpointRestoreRetainsPendingCleanup covers the pending-cleanup case; the metrics case has no new dedicated test. 5. CheckpointCoordinator flag: JobMaster.initCheckPointManager() already passes jobImmutableInformation.isRestoreJob() || restart to CheckpointManager, which forwards it to CheckpointCoordinator. That call site was not changed; this PR renames the constructor parameter to reflect the existing value, so it does not introduce new checkpoint-restore behavior. 6. Null handling: My previous explanation was incomplete. On the current head, CoordinatorService.submitJob has no restoreMode null guard followed by an unconditional dereference; it uses the boolean parameter described above. Separately, JobImmutableInformation.readData() initializes restoreMode from the legacy savepoint flag before downstream code reads a legacy payload. I hope these code pointers resolve the remaining questions. -- 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]
