You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by rmetzger <gi...@git.apache.org> on 2015/11/26 12:29:46 UTC
[GitHub] flink pull request: [FLINK-2954] Add config parameter for passing ...
GitHub user rmetzger opened a pull request:
https://github.com/apache/flink/pull/1409
[FLINK-2954] Add config parameter for passing environment variables to YARN
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rmetzger/flink flink2954
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1409.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 #1409
----
commit 74044d75b931b217f33816a7c9ed6f2ab085cb07
Author: Robert Metzger <rm...@apache.org>
Date: 2015-11-26T10:47:25Z
[FLINK-2954] Add config parameter for passing environment variables to YARN
----
---
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: [FLINK-2954] Add config parameter for passing ...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1409#issuecomment-162174050
Thanks for the review. I'll merge it soon
---
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: [FLINK-2954] Add config parameter for passing ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1409
---
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: [FLINK-2954] Add config parameter for passing ...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1409#discussion_r45973237
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/Utils.java ---
@@ -221,4 +210,22 @@ public static void addToEnvironment(Map<String, String> environment,
private Utils() {
throw new RuntimeException();
}
+
+ /**
+ * Method to extract environment variables from the flinkConfiguration based on the given prefix String.
+ *
+ * @param envPrefix Prefix for the environment variables key
+ * @param flinkConfiguration The Flink config to get the environment variable defintion from
+ */
+ public static Map<String, String> getEnvironmentVariables(String envPrefix, org.apache.flink.configuration.Configuration flinkConfiguration) {
+ Map<String, String> result = new HashMap<>();
+ for(Map.Entry<String, String> entry: flinkConfiguration.toMap().entrySet()) {
+ if(entry.getKey().startsWith(envPrefix)) {
+ // remove prefix
+ String key = entry.getKey().substring(envPrefix.length());
--- End diff --
I agree. I'll harden the method.
---
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: [FLINK-2954] Add config parameter for passing ...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/1409#discussion_r45970338
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/ConfigConstants.java ---
@@ -243,6 +243,20 @@
*/
public static final String YARN_PROPERTIES_FILE_LOCATION = "yarn.properties-file.location";
+ /**
+ * Prefix for passing custom environment variables to Flink's ApplicationMaster (JobManager).
+ * For example for passing LD_LIBRARY_PATH as an env variable to the AppMaster, set:
+ * yarn.application-master.env.LD_LIBRARY_PATH: "/usr/lib/native"
+ * in the flink-conf.yaml.
+ */
+ public static final String YARN_APPLICATION_MASTER_ENV_PREFIX = "yarn.application-master.env.";
+
+ /**
+ * Similar to the {@see YARN_APPLICATION_MASTER_ENV_PREFIX}, this configuration prefix allows
+ * setting custom environment variables.
+ */
+ public static final String YARN_TASK_MANAGER_ENV_PREFIX = "yarn.taskmanager.env.";
--- End diff --
I wonder about the naming here. Maybe this should be `YARN_RESOURCE_MANAGER_ENV_PREFIX`? Or change `YARN_APPLICATION_MASTER_ENV_PREFIX` to `YARN_JOB_MANAGER_ENV_PREFIX`?
---
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: [FLINK-2954] Add config parameter for passing ...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/1409#discussion_r45970372
--- Diff: flink-yarn-tests/src/main/java/org/apache/flink/yarn/UtilsTest.java ---
@@ -97,6 +98,18 @@ public void tooMuchCutoff() {
Assert.assertEquals(0, Utils.calculateHeapSize(4000, conf));
}
+ @Test
+ public void testGetEnvironmentVariables() {
+ Configuration testConf = new Configuration();
+ testConf.setString("yarn.application-master.env.LD_LIBRARY_PATH", "/usr/lib/native");
+
+ Map<String, String> res = Utils.getEnvironmentVariables("yarn.application-master.env.", testConf);
+
+ Assert.assertEquals(1, res.size());
+ Map.Entry<String, String> entry = res.entrySet().iterator().next();
+ Assert.assertEquals("LD_LIBRARY_PATH", entry.getKey());
+ Assert.assertEquals("/usr/lib/native", entry.getValue());
+ }
--- End diff --
There is no test for the task manager variables.
---
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: [FLINK-2954] Add config parameter for passing ...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/1409#issuecomment-159893248
Looks good except for some minor issues.
---
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: [FLINK-2954] Add config parameter for passing ...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/1409#discussion_r45970591
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/Utils.java ---
@@ -221,4 +210,22 @@ public static void addToEnvironment(Map<String, String> environment,
private Utils() {
throw new RuntimeException();
}
+
+ /**
+ * Method to extract environment variables from the flinkConfiguration based on the given prefix String.
+ *
+ * @param envPrefix Prefix for the environment variables key
+ * @param flinkConfiguration The Flink config to get the environment variable defintion from
+ */
+ public static Map<String, String> getEnvironmentVariables(String envPrefix, org.apache.flink.configuration.Configuration flinkConfiguration) {
+ Map<String, String> result = new HashMap<>();
+ for(Map.Entry<String, String> entry: flinkConfiguration.toMap().entrySet()) {
+ if(entry.getKey().startsWith(envPrefix)) {
+ // remove prefix
+ String key = entry.getKey().substring(envPrefix.length());
--- End diff --
What happens if the key is `envPrefix`? There is little validation 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: [FLINK-2954] Add config parameter for passing ...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1409#discussion_r45973129
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/ConfigConstants.java ---
@@ -243,6 +243,20 @@
*/
public static final String YARN_PROPERTIES_FILE_LOCATION = "yarn.properties-file.location";
+ /**
+ * Prefix for passing custom environment variables to Flink's ApplicationMaster (JobManager).
+ * For example for passing LD_LIBRARY_PATH as an env variable to the AppMaster, set:
+ * yarn.application-master.env.LD_LIBRARY_PATH: "/usr/lib/native"
+ * in the flink-conf.yaml.
+ */
+ public static final String YARN_APPLICATION_MASTER_ENV_PREFIX = "yarn.application-master.env.";
+
+ /**
+ * Similar to the {@see YARN_APPLICATION_MASTER_ENV_PREFIX}, this configuration prefix allows
+ * setting custom environment variables.
+ */
+ public static final String YARN_TASK_MANAGER_ENV_PREFIX = "yarn.taskmanager.env.";
--- End diff --
I think `YARN_RESOURCE_MANAGER_ENV_PREFIX` is just wrong bc its a service of YARN, not of Flink (http://hortonworks.com/blog/apache-hadoop-yarn-resourcemanager/).
Whether to use AM or JM is a good question. In the YARN context, AM and JM are basically the same. I think AM is a bit more consistent.
---
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: [FLINK-2954] Add config parameter for passing ...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1409#discussion_r45973217
--- Diff: flink-yarn-tests/src/main/java/org/apache/flink/yarn/UtilsTest.java ---
@@ -97,6 +98,18 @@ public void tooMuchCutoff() {
Assert.assertEquals(0, Utils.calculateHeapSize(4000, conf));
}
+ @Test
+ public void testGetEnvironmentVariables() {
+ Configuration testConf = new Configuration();
+ testConf.setString("yarn.application-master.env.LD_LIBRARY_PATH", "/usr/lib/native");
+
+ Map<String, String> res = Utils.getEnvironmentVariables("yarn.application-master.env.", testConf);
+
+ Assert.assertEquals(1, res.size());
+ Map.Entry<String, String> entry = res.entrySet().iterator().next();
+ Assert.assertEquals("LD_LIBRARY_PATH", entry.getKey());
+ Assert.assertEquals("/usr/lib/native", entry.getValue());
+ }
--- End diff --
I think its okay to assume that the method will work for all prefix strings.
This test is only about the method for extracting the variables.
---
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: [FLINK-2954] Add config parameter for passing ...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1409#issuecomment-159907627
Thank you for the good review. I addressed some of your concerns.
---
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.
---