kl0u commented on a change in pull request #13554:
URL: https://github.com/apache/flink/pull/13554#discussion_r512012032
##########
File path:
flink-clients/src/main/java/org/apache/flink/client/cli/GenericCLI.java
##########
@@ -105,44 +87,33 @@ public void addRunOptions(Options baseOptions) {
public void addGeneralOptions(Options baseOptions) {
baseOptions.addOption(executorOption);
baseOptions.addOption(targetOption);
- baseOptions.addOption(dynamicProperties);
+ baseOptions.addOption(DynamicPropertiesUtil.DYNAMIC_PROPERTIES);
}
@Override
- public Configuration applyCommandLineOptionsToConfiguration(final
CommandLine commandLine) {
- final Configuration effectiveConfiguration = new
Configuration(baseConfiguration);
+ public Configuration toConfiguration(final CommandLine commandLine) {
+ final Configuration resultConfiguration = new Configuration();
+
+ if
(configuration.getOptional(DeploymentOptions.TARGET).isPresent()) {
Review comment:
Can you explain a bit why we do not remove this `if() {}` block? If it
is in the base configuration, we will add it later, right?
##########
File path:
flink-kubernetes/src/main/java/org/apache/flink/kubernetes/cli/KubernetesSessionCli.java
##########
@@ -81,7 +81,8 @@ public KubernetesSessionCli(Configuration configuration,
ClusterClientServiceLoa
public Configuration getEffectiveConfiguration(String[] args) throws
CliArgsException {
Review comment:
This can now become package private and be annotated as
`@VisibleForTesting` I think.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/local/ExecutionContext.java
##########
@@ -341,8 +341,7 @@ private static Configuration createExecutionConfig(
availableCommandLines,
activeCommandLine);
- Configuration executionConfig =
activeCommandLine.applyCommandLineOptionsToConfiguration(
- commandLine);
+ Configuration executionConfig =
activeCommandLine.toConfiguration(commandLine);
Review comment:
I am not sure if here now we are missing the options from the base
configuration. I will check further but I am also writing it as a comment so
that you can also have a look.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
