minxhe opened a new pull request, #29109:
URL: https://github.com/apache/flink/pull/29109
# PR description — ready to paste
**Target:** `apache/flink` `master`
**Title:** `[FLINK-40559][clients] Honor --claimMode in application mode`
---
## 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 to `NO_CLAIM` with
no warning.
There are two independent causes:
1. `StandaloneApplicationClusterConfigurationParserFactory.getOptions()`
never registers
`SAVEPOINT_CLAIM_MODE` / `SAVEPOINT_RESTORE_MODE`. The parser runs with
`stopAtNonOption = true`, so an unrecognized `--claimMode` does not fail
— it ends
option parsing and the remainder is swallowed into `getArgs()`.
2. `CliFrontendParser.createSavepointRestoreSettings()` reads the claim mode
only inside
the branch guarded by `--fromSavepoint`. Passing `--claimMode` without a
savepoint
path returns `SavepointRestoreSettings.none()` and drops it.
Application-mode
JobManagers 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](https://issues.apache.org/jira/browse/FLINK-39673) /
[PR #28295](https://github.com/apache/flink/pull/28295), an unset claim
mode is no longer written to the `Configuration`, so a value from
`flink-conf.yaml`
survives. The remaining defect is that the **command-line option itself**
does not work.
## Brief change log
- `StandaloneApplicationClusterConfigurationParserFactory` — register
`SAVEPOINT_CLAIM_MODE` and `SAVEPOINT_RESTORE_MODE`.
- `CliFrontendParser` — branch on option presence rather than on the
presence of a
savepoint path; extract `parseRecoveryClaimMode()`. The parsed mode is
null exactly
when neither option was passed, so an explicit `--claimMode NO_CLAIM` stays
distinguishable from no override at all.
- `SavepointRestoreSettings` — add `forRecoveryClaimMode(...)` for a claim
mode with no
restore path, and extend `toString()` to render that state (it previously
printed
`none()`).
`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 (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:
1. `ProgramOptions` calls `createSavepointRestoreSettings()` and receives a
pathless
settings object containing the explicitly requested claim mode.
2. `ProgramOptions.applyToConfiguration()` writes that mode to
`RESTORE_MODE`, but no
`SAVEPOINT_PATH`.
3. `ExecutionConfigAccessor.getSavepointRestoreSettings()` reconstructs the
object
through `SavepointRestoreSettings.fromConfiguration()`.
4. `fromConfiguration()` returns `none()` as soon as `SAVEPOINT_PATH` is
absent, before
reading `RESTORE_MODE`, so the mode is lost on the round trip.
Application mode bypasses this path:
`StandaloneApplicationClusterConfigurationParserFactory` retains the parsed
settings
object, and
`StandaloneApplicationClusterEntryPoint.loadConfigurationFromClusterConfig()`
passes it directly to `toConfiguration()`.
Changing `fromConfiguration()` would affect the general session-mode CLI and
other
configuration 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(...)` when `RESTORE_MODE` is explicitly present
without a
`SAVEPOINT_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) —
`--claimMode` and
`--restoreMode` are picked up by the application-mode parser, i.e. the
options are
registered.
- `CliFrontendRunTest` (+3) — alongside the existing claim-mode coverage,
`--claimMode`
and `-rm` without `--fromSavepoint`, and a guard that passing neither
option still
yields `none()`. The `NO_CLAIM` case asserts on key *presence* in the
`Configuration`,
since an explicitly requested mode must be written even when it equals the
default.
- `SavepointRestoreSettingsTest` (+2) — the new factory, including that a
non-explicitly-set `allowNonRestoredState` is not written.
Both defects were first reproduced as failing tests on unmodified `master`
(`expected: CLAIM but was: NO_CLAIM`) and confirmed independent: registering
the options
fixes the with-savepoint case while the without-savepoint case still fails.
Local runs (JDK 17):
```
./mvnw -pl flink-container test Tests run: 13, Failures: 0, Errors: 0
./mvnw -pl flink-clients test Tests run: 292, Failures: 0, Errors: 0
./mvnw -pl flink-runtime -Dtest='org.apache.flink.runtime.jobgraph.**' test
Tests run: 63, Failures: 0, Errors: 0
./mvnw -pl flink-runtime,flink-clients,flink-container spotless:apply
checkstyle:check
0 Checkstyle violations
```
`createSavepointRestoreSettings` has only two callers — `ProgramOptions` and
`StandaloneApplicationClusterConfigurationParserFactory` — both covered
above. Each
commit was verified green on its own.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): **no**
- The public API, i.e., is any changed class annotated with
`@Public(Evolving)`: **no**
- The serializers: **no** (no fields added; `serialVersionUID` unchanged)
- The runtime per-record code paths (performance sensitive): **no** (CLI
parsing at
startup only)
- Anything that affects deployment or recovery: JobManager (and its
components),
Checkpointing, Kubernetes/Yarn, ZooKeeper: **yes** — changes how
`--claimMode`
reaches the JobManager `Configuration` at startup, which determines the
recovery
claim 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.
- The S3 file system connector: **no**
## Documentation
- Does this pull request introduce a new feature? **no** — it fixes an
existing CLI
option that was silently ignored.
- If yes, how is the feature documented? **not applicable**
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: GitHub Copilot CLI 1.0.80
--
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]
