kl0u commented on a change in pull request #13554:
URL: https://github.com/apache/flink/pull/13554#discussion_r501550609
##########
File path:
flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontend.java
##########
@@ -892,15 +895,20 @@ private JobID parseJobId(String jobIdString) throws
CliArgsException {
* @throws FlinkException if something goes wrong
*/
private <ClusterID> void runClusterAction(CustomCommandLine
activeCommandLine, CommandLine commandLine, ClusterAction<ClusterID>
clusterAction) throws FlinkException {
- final Configuration executorConfig =
activeCommandLine.applyCommandLineOptionsToConfiguration(commandLine);
- final ClusterClientFactory<ClusterID> clusterClientFactory =
clusterClientServiceLoader.getClusterClientFactory(executorConfig);
+ final Configuration effectiveConfiguration = new
Configuration(configuration);
+ final Configuration commandLineConfiguration =
activeCommandLine.toConfiguration(commandLine);
+ effectiveConfiguration.addAll(commandLineConfiguration);
- final ClusterID clusterId =
clusterClientFactory.getClusterId(executorConfig);
+ LOG.debug("Effective configuration: {}",
effectiveConfiguration);
Review comment:
Given that the `env` itself can add config options later, maybe the
message here could change to reflect that this is the `effective configuration
at the CLI client` or `after the addition of the CLI options`. Although a bit
esoteric, this is a debug message so I think it is ok. If we want the actual
effective config I think we should go in the `env.executeAsync()`.
##########
File path:
flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontend.java
##########
@@ -1001,9 +1009,10 @@ public int parseParameters(String[] args) {
return handleParametrizationException(ppe);
} catch (ProgramMissingJobException pmje) {
return handleMissingJobException();
- } catch (Exception e) {
- return handleError(e);
}
+// catch (Exception e) {
Review comment:
Shouldn't this be either removed or uncommented?
##########
File path:
flink-clients/src/main/java/org/apache/flink/client/cli/GenericCLI.java
##########
@@ -109,27 +104,29 @@ public void addGeneralOptions(Options baseOptions) {
}
@Override
- public Configuration applyCommandLineOptionsToConfiguration(final
CommandLine commandLine) {
- final Configuration effectiveConfiguration = new
Configuration(baseConfiguration);
+ public Configuration toConfiguration(final CommandLine commandLine) {
+ final Configuration resultConfiguration = new Configuration();
+
Review comment:
Do we need this `if() {...}`? Eventually we will merge the configuration
passed at the constructor to the one returned here, right? In addition, getting
rid of accessing the outside configuration from here is one of the main points
of this work, right?
##########
File path:
flink-clients/src/main/java/org/apache/flink/client/cli/DefaultCLI.java
##########
@@ -37,16 +38,24 @@
*/
public class DefaultCLI extends AbstractCustomCommandLine {
+ public static final String ID = "default";
+
private static final Option addressOption = new Option("m",
"jobmanager", true,
"Address of the JobManager to which to connect. " +
"Use this flag to connect to a different JobManager
than the one specified in the configuration. " +
"Attention: This option is respected only if the
high-availability configuration is NONE.");
- public static final String ID = "default";
-
- public DefaultCLI(Configuration configuration) {
- super(configuration);
- }
+ /**
+ * Dynamic properties allow the user to specify additional
configuration values with -D, such as
+ * <tt> -Dfs.overwrite-files=true
-Dtaskmanager.memory.network.min=536346624</tt>.
+ */
Review comment:
If I am not mistaken, now all `CustomCommandLines` have dynamic options.
So why not moving the option and the encode/decode logic to the base class or a
utility class (for the encode/decode)?
Actually, I believe that this will bring the `Generic` and the `Default CLI`
codebases even closer.
----------------------------------------------------------------
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]
