You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tillrohrmann <gi...@git.apache.org> on 2017/07/07 11:42:46 UTC
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
GitHub user tillrohrmann opened a pull request:
https://github.com/apache/flink/pull/4280
[FLINK-7125] [yarn] Remove Configuration loading from AbstractYarnClusterDescriptor
Instead the AbstractYarnClusterDescriptor is passed in a Configuration instance which
is sent to the started application master.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tillrohrmann/flink removeConfigLoadingFromYarnClusterDescriptor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4280.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4280
----
commit 0ea989cfac656cc1063411db9043d861bc8be39b
Author: Till Rohrmann <tr...@apache.org>
Date: 2017-07-07T11:40:29Z
[FLINK-7125] [yarn] Remove Configuration loading from AbstractYarnClusterDescriptor
Instead the AbstractYarnClusterDescriptor is passed in a Configuration instance which
is sent to the started application master.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r130032185
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java ---
@@ -478,12 +485,15 @@ public static void runInteractiveCli(YarnClusterClient yarnCluster, boolean read
public static void main(final String[] args) throws Exception {
final FlinkYarnSessionCli cli = new FlinkYarnSessionCli("", ""); // no prefix for the YARN session
- Configuration flinkConfiguration = GlobalConfiguration.loadConfiguration();
+
+ final String configurationDirectory = CliFrontend.getConfigurationDirectoryFromEnv();
--- End diff --
Yes it should. The CLIs behave differently mainly because of historical reasons (different time when they were developed).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129772021
--- Diff: flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java ---
@@ -549,7 +559,12 @@ protected void runWithArgs(String[] args, String terminateAfterString, String[]
final int startTimeoutSeconds = 180;
final long deadline = System.currentTimeMillis() + (startTimeoutSeconds * 1000);
- Runner runner = new Runner(args, type, expectedReturnValue);
+ Runner runner = new Runner(
+ args,
+ flinkConfiguration,
+ CliFrontend.getConfigurationDirectoryFromEnv(),
--- End diff --
ditto: shouldn't the directory come from `args`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129870736
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CustomCommandLine.java ---
@@ -62,25 +62,27 @@
* Retrieves a client for a running cluster.
* @param commandLine The command-line parameters from the CliFrontend
* @param config The Flink config
+ * @param configurationDirectory Direcotry for configuration files
* @return Client if a cluster could be retrieved
* @throws UnsupportedOperationException if the operation is not supported
*/
ClusterType retrieveCluster(
- CommandLine commandLine,
- Configuration config) throws UnsupportedOperationException;
+ CommandLine commandLine,
+ Configuration config,
+ String configurationDirectory) throws UnsupportedOperationException;
/**
* Creates the client for the cluster.
* @param applicationName The application name to use
* @param commandLine The command-line options parsed by the CliFrontend
* @param config The Flink config to use
- * @param userJarFiles User jar files to include in the classpath of the cluster.
- * @return The client to communicate with the cluster which the CustomCommandLine brought up.
+ * @param configurationDirectory
+ *@param userJarFiles User jar files to include in the classpath of the cluster. @return The client to communicate with the cluster which the CustomCommandLine brought up.
* @throws Exception if the cluster could not be created
*/
ClusterType createCluster(
- String applicationName,
- CommandLine commandLine,
- Configuration config,
- List<URL> userJarFiles) throws Exception;
+ String applicationName,
+ CommandLine commandLine,
+ Configuration config,
+ String configurationDirectory, List<URL> userJarFiles) throws Exception;
--- End diff --
Good catch. Will change it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4280
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129878926
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -722,12 +697,30 @@ public ApplicationReport startAppMaster(JobGraph jobGraph, YarnClient yarnClient
// Setup jar for ApplicationMaster
LocalResource appMasterJar = Records.newRecord(LocalResource.class);
- LocalResource flinkConf = Records.newRecord(LocalResource.class);
--- End diff --
Fair point. Will try to do it the next time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129875747
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnCLI.java ---
@@ -214,22 +218,21 @@ public void addGeneralOptions(Options baseOptions) {
@Override
public YarnClusterClientV2 retrieveCluster(
- CommandLine cmdLine,
- Configuration config) throws UnsupportedOperationException {
+ CommandLine cmdLine,
+ Configuration config, String configurationDirectory) throws UnsupportedOperationException {
throw new UnsupportedOperationException("Not support retrieveCluster since Flip-6.");
}
@Override
public YarnClusterClientV2 createCluster(
- String applicationName,
- CommandLine cmdLine,
- Configuration config,
- List<URL> userJarFiles) throws Exception {
+ String applicationName,
+ CommandLine cmdLine,
+ Configuration config,
+ String configurationDirectory, List<URL> userJarFiles) throws Exception {
--- End diff --
Done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129770769
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnCLI.java ---
@@ -214,22 +218,21 @@ public void addGeneralOptions(Options baseOptions) {
@Override
public YarnClusterClientV2 retrieveCluster(
- CommandLine cmdLine,
- Configuration config) throws UnsupportedOperationException {
+ CommandLine cmdLine,
+ Configuration config, String configurationDirectory) throws UnsupportedOperationException {
--- End diff --
nit: formatting
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129878461
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -163,23 +161,17 @@ public AbstractYarnClusterDescriptor() {
}
}
- // tries to load the config through the environment, if it fails it can still be set through the setters
- try {
- this.configurationDirectory = CliFrontend.getConfigurationDirectoryFromEnv();
- this.flinkConfiguration = GlobalConfiguration.loadConfiguration(configurationDirectory);
+ this.flinkConfiguration = Preconditions.checkNotNull(flinkConfiguration);
+ userJarInclusion = getUserJarInclusionMode(flinkConfiguration);
- File confFile = new File(configurationDirectory + File.separator + GlobalConfiguration.FLINK_CONF_FILENAME);
- if (!confFile.exists()) {
- throw new RuntimeException("Unable to locate configuration file in " + confFile);
- }
- flinkConfigurationPath = new Path(confFile.getAbsolutePath());
+ this.configurationDirectory = Preconditions.checkNotNull(configurationDirectory);
+ // tries to load the config through the environment, if it fails it can still be set through the setters
--- End diff --
I think it is no longer accurate. Good catch :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129769550
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/DefaultCLI.java ---
@@ -58,7 +58,7 @@ public void addGeneralOptions(Options baseOptions) {
}
@Override
- public StandaloneClusterClient retrieveCluster(CommandLine commandLine, Configuration config) {
+ public StandaloneClusterClient retrieveCluster(CommandLine commandLine, Configuration config, String configurationDirectory) {
--- End diff --
nit: maybe wrap params?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4280: [FLINK-7125] [yarn] Remove Configuration loading from Abs...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/4280
Thanks for your review @pnowojski. The PR passed on [Travis](https://travis-ci.org/tillrohrmann/flink/builds/258473756). Merging it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129875590
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnCLI.java ---
@@ -214,22 +218,21 @@ public void addGeneralOptions(Options baseOptions) {
@Override
public YarnClusterClientV2 retrieveCluster(
- CommandLine cmdLine,
- Configuration config) throws UnsupportedOperationException {
+ CommandLine cmdLine,
+ Configuration config, String configurationDirectory) throws UnsupportedOperationException {
--- End diff --
I shouldn't use automatic reformatting. It screws up the formatting ;-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129770778
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnCLI.java ---
@@ -214,22 +218,21 @@ public void addGeneralOptions(Options baseOptions) {
@Override
public YarnClusterClientV2 retrieveCluster(
- CommandLine cmdLine,
- Configuration config) throws UnsupportedOperationException {
+ CommandLine cmdLine,
+ Configuration config, String configurationDirectory) throws UnsupportedOperationException {
throw new UnsupportedOperationException("Not support retrieveCluster since Flip-6.");
}
@Override
public YarnClusterClientV2 createCluster(
- String applicationName,
- CommandLine cmdLine,
- Configuration config,
- List<URL> userJarFiles) throws Exception {
+ String applicationName,
+ CommandLine cmdLine,
+ Configuration config,
+ String configurationDirectory, List<URL> userJarFiles) throws Exception {
--- End diff --
nit: formatting
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129771505
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java ---
@@ -478,12 +485,15 @@ public static void runInteractiveCli(YarnClusterClient yarnCluster, boolean read
public static void main(final String[] args) throws Exception {
final FlinkYarnSessionCli cli = new FlinkYarnSessionCli("", ""); // no prefix for the YARN session
- Configuration flinkConfiguration = GlobalConfiguration.loadConfiguration();
+
+ final String configurationDirectory = CliFrontend.getConfigurationDirectoryFromEnv();
--- End diff --
Shouldn't the directory come from `args` with a default value taken from `env`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129879671
--- Diff: flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java ---
@@ -210,6 +207,7 @@ public void testSetupApplicationMasterContainer() {
.getCommands().get(0));
// logback + log4j, with/out krb5, different JVM opts
+ // IMPORTANT: Beaware that we are using side effects here to modify the created YarnClusterDescriptor
--- End diff --
We keep a reference on the `Configuration` object being held by the `ClusterDescriptor`. Thus, the different settings for the different assert statement end up in the same configuration instance. I'll add this to the comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129881025
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java ---
@@ -478,12 +485,15 @@ public static void runInteractiveCli(YarnClusterClient yarnCluster, boolean read
public static void main(final String[] args) throws Exception {
final FlinkYarnSessionCli cli = new FlinkYarnSessionCli("", ""); // no prefix for the YARN session
- Configuration flinkConfiguration = GlobalConfiguration.loadConfiguration();
+
+ final String configurationDirectory = CliFrontend.getConfigurationDirectoryFromEnv();
--- End diff --
Maybe it should? It is kind of strange that part of the configuration is passed through `args` and some through `env`. I have never used this cli so maybe I have wrong feeling about it :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129774051
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -163,23 +161,17 @@ public AbstractYarnClusterDescriptor() {
}
}
- // tries to load the config through the environment, if it fails it can still be set through the setters
- try {
- this.configurationDirectory = CliFrontend.getConfigurationDirectoryFromEnv();
- this.flinkConfiguration = GlobalConfiguration.loadConfiguration(configurationDirectory);
+ this.flinkConfiguration = Preconditions.checkNotNull(flinkConfiguration);
+ userJarInclusion = getUserJarInclusionMode(flinkConfiguration);
- File confFile = new File(configurationDirectory + File.separator + GlobalConfiguration.FLINK_CONF_FILENAME);
- if (!confFile.exists()) {
- throw new RuntimeException("Unable to locate configuration file in " + confFile);
- }
- flinkConfigurationPath = new Path(confFile.getAbsolutePath());
+ this.configurationDirectory = Preconditions.checkNotNull(configurationDirectory);
+ // tries to load the config through the environment, if it fails it can still be set through the setters
--- End diff --
is this comment still accurate?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129768986
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CustomCommandLine.java ---
@@ -62,25 +62,27 @@
* Retrieves a client for a running cluster.
* @param commandLine The command-line parameters from the CliFrontend
* @param config The Flink config
+ * @param configurationDirectory Direcotry for configuration files
* @return Client if a cluster could be retrieved
* @throws UnsupportedOperationException if the operation is not supported
*/
ClusterType retrieveCluster(
- CommandLine commandLine,
- Configuration config) throws UnsupportedOperationException;
+ CommandLine commandLine,
+ Configuration config,
+ String configurationDirectory) throws UnsupportedOperationException;
/**
* Creates the client for the cluster.
* @param applicationName The application name to use
* @param commandLine The command-line options parsed by the CliFrontend
* @param config The Flink config to use
- * @param userJarFiles User jar files to include in the classpath of the cluster.
- * @return The client to communicate with the cluster which the CustomCommandLine brought up.
+ * @param configurationDirectory
+ *@param userJarFiles User jar files to include in the classpath of the cluster. @return The client to communicate with the cluster which the CustomCommandLine brought up.
* @throws Exception if the cluster could not be created
*/
ClusterType createCluster(
- String applicationName,
- CommandLine commandLine,
- Configuration config,
- List<URL> userJarFiles) throws Exception;
+ String applicationName,
+ CommandLine commandLine,
+ Configuration config,
+ String configurationDirectory, List<URL> userJarFiles) throws Exception;
--- End diff --
nit: broken formatting
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129877902
--- Diff: flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java ---
@@ -549,7 +559,12 @@ protected void runWithArgs(String[] args, String terminateAfterString, String[]
final int startTimeoutSeconds = 180;
final long deadline = System.currentTimeMillis() + (startTimeoutSeconds * 1000);
- Runner runner = new Runner(args, type, expectedReturnValue);
+ Runner runner = new Runner(
+ args,
+ flinkConfiguration,
+ CliFrontend.getConfigurationDirectoryFromEnv(),
--- End diff --
Since the configuration directory is only required for the session case in `Runner`, the same argument applies as above. I agree that the `FlinkYarnSessionCli` should have a configurable configuration directory which can be controlled by a command line option.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129769520
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/DefaultCLI.java ---
@@ -77,10 +77,10 @@ public StandaloneClusterClient retrieveCluster(CommandLine commandLine, Configur
@Override
public StandaloneClusterClient createCluster(
- String applicationName,
- CommandLine commandLine,
- Configuration config,
- List<URL> userJarFiles) throws UnsupportedOperationException {
+ String applicationName,
+ CommandLine commandLine,
+ Configuration config,
+ String configurationDirectory, List<URL> userJarFiles) throws UnsupportedOperationException {
--- End diff --
nit: formatting
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129777841
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java ---
@@ -722,12 +697,30 @@ public ApplicationReport startAppMaster(JobGraph jobGraph, YarnClient yarnClient
// Setup jar for ApplicationMaster
LocalResource appMasterJar = Records.newRecord(LocalResource.class);
- LocalResource flinkConf = Records.newRecord(LocalResource.class);
--- End diff --
nit: for me it would be easier to understand/review the changes done here, if there was a separate PR for changing formatting (splitting one param per line)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129774438
--- Diff: flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java ---
@@ -210,6 +207,7 @@ public void testSetupApplicationMasterContainer() {
.getCommands().get(0));
// logback + log4j, with/out krb5, different JVM opts
+ // IMPORTANT: Beaware that we are using side effects here to modify the created YarnClusterDescriptor
--- End diff --
what are those side effects?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129876779
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java ---
@@ -478,12 +485,15 @@ public static void runInteractiveCli(YarnClusterClient yarnCluster, boolean read
public static void main(final String[] args) throws Exception {
final FlinkYarnSessionCli cli = new FlinkYarnSessionCli("", ""); // no prefix for the YARN session
- Configuration flinkConfiguration = GlobalConfiguration.loadConfiguration();
+
+ final String configurationDirectory = CliFrontend.getConfigurationDirectoryFromEnv();
--- End diff --
The `FlinkYarnSessionCli` does not support a command line option to specify a configuration directory. Therefore, this is not needed here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129875395
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/DefaultCLI.java ---
@@ -77,10 +77,10 @@ public StandaloneClusterClient retrieveCluster(CommandLine commandLine, Configur
@Override
public StandaloneClusterClient createCluster(
- String applicationName,
- CommandLine commandLine,
- Configuration config,
- List<URL> userJarFiles) throws UnsupportedOperationException {
+ String applicationName,
+ CommandLine commandLine,
+ Configuration config,
+ String configurationDirectory, List<URL> userJarFiles) throws UnsupportedOperationException {
--- End diff --
Done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4280: [FLINK-7125] [yarn] Remove Configuration loading f...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4280#discussion_r129875326
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/DefaultCLI.java ---
@@ -58,7 +58,7 @@ public void addGeneralOptions(Options baseOptions) {
}
@Override
- public StandaloneClusterClient retrieveCluster(CommandLine commandLine, Configuration config) {
+ public StandaloneClusterClient retrieveCluster(CommandLine commandLine, Configuration config, String configurationDirectory) {
--- End diff --
Yes, will do.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---