[FLINK-40559][clients] Honor --claimMode in application mode - #29109
Open
minxhe wants to merge 3 commits into
Open
[FLINK-40559][clients] Honor --claimMode in application mode#29109minxhe wants to merge 3 commits into
minxhe wants to merge 3 commits into
Conversation
…ation mode StandaloneApplicationClusterConfigurationParserFactory.getOptions() did not register SAVEPOINT_CLAIM_MODE or SAVEPOINT_RESTORE_MODE. Because the parser runs with stopAtNonOption=true, an unrecognized --claimMode did not fail: it ended option parsing and the remainder was swallowed into getArgs(). The claim mode was therefore unreachable in application mode and fell back to NO_CLAIM. Register both options so that createSavepointRestoreSettings can read them. Behavior is unchanged for jobs that pass neither option. Generated-by: GitHub Copilot CLI 1.0.80
createSavepointRestoreSettings read the claim mode only inside the branch guarded by --fromSavepoint, so passing --claimMode without a savepoint path returned SavepointRestoreSettings.none() and discarded it. Application-mode JobManagers hit this on HA recovery, where the checkpoint comes from HA storage rather than from a command line path. Branch on option presence instead, and add SavepointRestoreSettings.forRecoveryClaimMode for a claim mode with no restore path. The parsed mode is null exactly when neither option was passed, so an explicit --claimMode NO_CLAIM stays distinguishable from no override at all. allowNonRestoredState stays null unless the flag was passed, so the new branch does not start writing execution.state-recovery.ignore-unclaimed-state where nothing was written before (FLINK-39673). SavepointRestoreSettings.fromConfiguration has the same savepoint-path gate, so the `flink run` path still drops a pathless claim mode. That asymmetry is pre-existing and left untouched here; application mode writes directly to the JobManager configuration and does not go through it. Generated-by: GitHub Copilot CLI 1.0.80
Contributor
|
Can you remove the first section of the PR description. |
spuru9
suggested changes
Sep 5, 2026
Generated-by: GitHub Copilot CLI 1.0.83-5 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Author
thanks for the catch, have removed! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
Flink silently discards an explicitly-passed
--claimMode(or its deprecated alias--restoreMode), so the recovery claim mode falls back toNO_CLAIMwith no warning.There are two independent causes:
StandaloneApplicationClusterConfigurationParserFactory.getOptions()never registersSAVEPOINT_CLAIM_MODE/SAVEPOINT_RESTORE_MODE. The parser runs withstopAtNonOption = true, so an unrecognized--claimModedoes not fail — it endsoption parsing and the remainder is swallowed into
getArgs().CliFrontendParser.createSavepointRestoreSettings()reads the claim mode only insidethe branch guarded by
--fromSavepoint. Passing--claimModewithout a savepointpath returns
SavepointRestoreSettings.none()and drops it. Application-modeJobManagers hit this on HA recovery, where the checkpoint comes from HA storage
rather than from a command line path.
This is not about configuration being overwritten. Since
FLINK-39673 /
PR #28295, an unset claim
mode is no longer written to the
Configuration, so a value fromflink-conf.yamlsurvives. The remaining defect is that the command-line option itself does not work.
Brief change log
StandaloneApplicationClusterConfigurationParserFactory— registerSAVEPOINT_CLAIM_MODEandSAVEPOINT_RESTORE_MODE.CliFrontendParser— branch on option presence rather than on the presence of asavepoint path; extract
parseRecoveryClaimMode(). The parsed mode is null exactlywhen neither option was passed, so an explicit
--claimMode NO_CLAIMstaysdistinguishable from no override at all.
SavepointRestoreSettings— addforRecoveryClaimMode(...)for a claim mode with norestore path, and extend
toString()to render that state (it previously printednone()).allowNonRestoredStatestaysnullunless the flag was passed, so the new branch doesnot start writing
execution.state-recovery.ignore-unclaimed-statewhere nothing waswritten before (the "explicitly set" semantics from FLINK-39673).
Split into two commits so the option registration can be reviewed independently of the
parser restructure.
Known limitation, deliberately out of scope
There is a separate round-trip asymmetry in session-mode job submission:
ProgramOptionscallscreateSavepointRestoreSettings()and receives a pathlesssettings object containing the explicitly requested claim mode.
ProgramOptions.applyToConfiguration()writes that mode toRESTORE_MODE, but noSAVEPOINT_PATH.ExecutionConfigAccessor.getSavepointRestoreSettings()reconstructs the objectthrough
SavepointRestoreSettings.fromConfiguration().fromConfiguration()returnsnone()as soon asSAVEPOINT_PATHis absent, beforereading
RESTORE_MODE, so the mode is lost on the round trip.Application mode bypasses this path:
StandaloneApplicationClusterConfigurationParserFactoryretains the parsed settingsobject, and
StandaloneApplicationClusterEntryPoint.loadConfigurationFromClusterConfig()passes it directly to
toConfiguration().Changing
fromConfiguration()would affect the general session-mode CLI and otherconfiguration consumers. Because the CLI option is documented in terms of restoring from
a given savepoint, the intended semantics of a pathless claim mode for regular
session-mode submission should be agreed separately. A follow-up could return
forRecoveryClaimMode(...)whenRESTORE_MODEis explicitly present without aSAVEPOINT_PATH.This PR therefore limits the change to application-mode startup. Happy to include the
round-trip change here or file a follow-up if reviewers consider it part of the general
CLI contract.
Verifying this change
This change added tests and can be verified as follows:
StandaloneApplicationClusterConfigurationParserFactoryTest(+2) —--claimModeand--restoreModeare picked up by the application-mode parser, i.e. the options areregistered.
CliFrontendRunTest(+3) — alongside the existing claim-mode coverage,--claimModeand
-rmwithout--fromSavepoint, and a guard that passing neither option stillyields
none(). TheNO_CLAIMcase asserts on key presence in theConfiguration,since an explicitly requested mode must be written even when it equals the default.
SavepointRestoreSettingsTest(+2) — the new factory, including that anon-explicitly-set
allowNonRestoredStateis not written.Both defects were first reproduced as failing tests on unmodified
master(
expected: CLAIM but was: NO_CLAIM) and confirmed independent: registering the optionsfixes the with-savepoint case while the without-savepoint case still fails.
Local runs (JDK 17):
createSavepointRestoreSettingshas only two callers —ProgramOptionsandStandaloneApplicationClusterConfigurationParserFactory— both covered above. Eachcommit was verified green on its own.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noserialVersionUIDunchanged)startup only)
Checkpointing, Kubernetes/Yarn, ZooKeeper: yes — changes how
--claimModereaches the JobManager
Configurationat startup, which determines the recoveryclaim mode. Inert for jobs that do not pass the option; for jobs that do, the
behavior changes from silently ignoring it to honoring it, which is the fix.
Documentation
option that was silently ignored.
Was generative AI tooling used to co-author this PR?
Generated-by: GitHub Copilot CLI 1.0.80