masteryhx commented on code in PR #24058:
URL: https://github.com/apache/flink/pull/24058#discussion_r1448208494
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/SavepointRestoreSettings.java:
##########
@@ -163,14 +163,16 @@ public static SavepointRestoreSettings forPath(
public static void toConfiguration(
final SavepointRestoreSettings savepointRestoreSettings,
final Configuration configuration) {
- configuration.set(
- SavepointConfigOptions.SAVEPOINT_IGNORE_UNCLAIMED_STATE,
- savepointRestoreSettings.allowNonRestoredState());
- configuration.set(
- SavepointConfigOptions.RESTORE_MODE,
savepointRestoreSettings.getRestoreMode());
- final String savepointPath = savepointRestoreSettings.getRestorePath();
- if (savepointPath != null) {
- configuration.setString(SavepointConfigOptions.SAVEPOINT_PATH,
savepointPath);
+ if (!savepointRestoreSettings.equals(SavepointRestoreSettings.none()))
{
Review Comment:
I saw the equals of savepointRestoreSettings is not correct, so I created
https://github.com/apache/flink/pull/24066 to fix this at first, welcome to
review.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/SavepointRestoreSettings.java:
##########
@@ -163,14 +163,16 @@ public static SavepointRestoreSettings forPath(
public static void toConfiguration(
final SavepointRestoreSettings savepointRestoreSettings,
final Configuration configuration) {
- configuration.set(
- SavepointConfigOptions.SAVEPOINT_IGNORE_UNCLAIMED_STATE,
- savepointRestoreSettings.allowNonRestoredState());
- configuration.set(
- SavepointConfigOptions.RESTORE_MODE,
savepointRestoreSettings.getRestoreMode());
- final String savepointPath = savepointRestoreSettings.getRestorePath();
- if (savepointPath != null) {
- configuration.setString(SavepointConfigOptions.SAVEPOINT_PATH,
savepointPath);
+ if (!savepointRestoreSettings.equals(SavepointRestoreSettings.none()))
{
Review Comment:
I think the method contains the logic about setting savepointRestoreSettings
to Configuration even for SavepointRestoreSettings.none.
Some callers may just want to use SavepointRestoreSettings.none to override
the configuration. e.g. RemoteStreamEnvironment#getEffectiveConfiguration (Of
course, it seems not so correct about its logic).
So How about letting the caller decide whether toConfiguration ? WDYT?
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
