lincoln-lil commented on code in PR #28295:
URL: https://github.com/apache/flink/pull/28295#discussion_r3377656741
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/SavepointRestoreSettings.java:
##########
@@ -147,24 +150,24 @@ public static SavepointRestoreSettings none() {
}
public static SavepointRestoreSettings forPath(String savepointPath) {
- return forPath(
- savepointPath,
-
StateRecoveryOptions.SAVEPOINT_IGNORE_UNCLAIMED_STATE.defaultValue());
+ return forPath(savepointPath, null);
}
public static SavepointRestoreSettings forPath(
- String savepointPath, boolean allowNonRestoredState) {
- checkNotNull(savepointPath, "Savepoint restore path.");
- return new SavepointRestoreSettings(
- savepointPath,
- allowNonRestoredState,
- StateRecoveryOptions.RESTORE_MODE.defaultValue());
+ String savepointPath, @Nullable Boolean allowNonRestoredState) {
+ return new SavepointRestoreSettings(savepointPath,
allowNonRestoredState, null);
Review Comment:
nit: since `checkNotNull` was removed, the `savepointPath` should also add
`@Nullable` annotation？
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/SavepointRestoreSettings.java:
##########
@@ -37,30 +38,31 @@ public class SavepointRestoreSettings implements
Serializable {
/** No restore should happen. */
private static final SavepointRestoreSettings NONE =
- new SavepointRestoreSettings(null, false,
RecoveryClaimMode.NO_CLAIM);
+ new SavepointRestoreSettings(null, null, null);
/** Savepoint restore path. */
- private final String restorePath;
+ private final @Nullable String restorePath;
/**
* Flag indicating whether non restored state is allowed if the savepoint
contains state for an
* operator that is not part of the job.
*/
- private final boolean allowNonRestoredState;
+ private final @Nullable Boolean allowNonRestoredState;
Review 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?
--
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]
