[FLINK-36973][config] Skip options not explicitly set when converting SavepointRestoreSettings to Configuration#28295
[FLINK-36973][config] Skip options not explicitly set when converting SavepointRestoreSettings to Configuration#28295AlexYinHan wants to merge 1 commit into
Conversation
AlexYinHan
force-pushed
the
yh/dev_39673
branch
5 times, most recently
from
e128b73 to
6fa1a89
Compare
lincoln-lil
left a comment
lincoln-lil
left a comment
There was a problem hiding this comment.
@AlexYinHan Thanks for fixing this! I've left two comments there, PTAL.
| * operator that is not part of the job. | ||
| */ | ||
| private final boolean allowNonRestoredState; | ||
| private final @Nullable Boolean allowNonRestoredState; |
There was a problem hiding this comment.
Is the boolean → Boolean change necessary? This may produces incompatible Java serialization. Since SavepointRestoreSettings is embedded in JobGraph and Java-serialized occurs during e.g., session cluster submission, this may silently drops field values in mixed-version clusters (e.g., new cli → old server). The unchanged serialVersionUID suppresses InvalidClassException, making the failure silent.
So could we either update the UID or keep primitive fields and track explicitly set separately?
There was a problem hiding this comment.
Thanks for pointing it out. I forgot to consider the serialization problem here. I keep the primitive boolean field and add a allowNonRestoredStateExplicitlySet to track it separately.
| allowNonRestoredState, | ||
| StateRecoveryOptions.RESTORE_MODE.defaultValue()); | ||
| String savepointPath, @Nullable Boolean allowNonRestoredState) { | ||
| return new SavepointRestoreSettings(savepointPath, allowNonRestoredState, null); |
There was a problem hiding this comment.
nit: since checkNotNull was removed, the savepointPath should also add @Nullable annotation？
There was a problem hiding this comment.
Sure. All forPath methods now either call checkNotNull or add @Nullable annotation.
… SavepointRestoreSettings to Configuration
|
@lincoln-lil Thanks for your comments. I have modified the code accordingly. PTAL :) |
|
LGTM from my side. Could you also have a look when you get a chance? cc @kl0u |
3 participants
What is the purpose of the change
This PR fixes the issue in FLINK-39673. It skips writing
RESTORE_MODEandSAVEPOINT_IGNORE_UNCLAIMED_STATEfromSavepointRestoreSettingsintoConfigurationwhen they were not explicitly provided. If the user uses deprecated configuration keys for these options, this prevents the configurations from being overridden by default values.Brief change log
SavepointRestoreSettings, change theallowNonRestoredStateandrecoveryClaimModefromNonNulltoNullable, to tell whether they are explicitly provided.SavepointRestoreSettings.toConfiguration()to skip writingRESTORE_MODEandSAVEPOINT_IGNORE_UNCLAIMED_STATEwhen they were not explicitly provided by the user.Verifying this change
This change added tests and can be verified as follows:
SavepointRestoreSettingsTest.javaAdded test that validates that default/null values of SavepointRestoreSettings are correctly handledDoes this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation
Was generative AI tooling used to co-author this PR?
Generated-by: [Claude-4.6-Opus]