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/06/30 18:23:33 UTC
[GitHub] flink pull request: [FLINK-2298] Allow setting a custom applicatio...
GitHub user rmetzger opened a pull request:
https://github.com/apache/flink/pull/876
[FLINK-2298] Allow setting a custom application name on YARN
With this change, users can pass a "--name" argument to the YARN session to give the YARN application a custom name.
I've covered the change in a test case and tested it on HDP 2.3 (with hadoop 2.7.1, which we apparently also support ;) )
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rmetzger/flink flink2298
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/876.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 #876
----
commit 2db26e4b212e3539c8df749bfbfce6d1b077a361
Author: Robert Metzger <rm...@apache.org>
Date: 2015-06-30T14:56:30Z
[FLINK-2298] Add option to pass a custom name for Flink on 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-2298] Allow setting a custom applicatio...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/876#discussion_r33657185
--- Diff: flink-yarn-tests/src/main/java/org/apache/flink/yarn/YARNSessionFIFOITCase.java ---
@@ -121,6 +121,7 @@ public void testDetachedMode() {
"-n", "1",
"-jm", "768",
"-tm", "1024",
+ "--name", "MyCustomName", // test setting a custom name
--- End diff --
There is no test for the short option `-nm` which I would like to remove.
---
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-2298] Allow setting a custom applicatio...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/876#issuecomment-117581828
I've addressed your concerns and updated the PR.
---
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-2298] Allow setting a custom applicatio...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/876#discussion_r33662529
--- Diff: docs/setup/yarn_setup.md ---
@@ -100,11 +100,13 @@ Usage:
-D <arg> Dynamic properties
-d,--detached Start detached
-jm,--jobManagerMemory <arg> Memory for JobManager Container [in MB]
+ -nm,--name Set a custom name for the application on YARN
--- End diff --
Fair point. I don't think it's necessary but if it is for the sake of user satisfaction you have my +1.
---
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-2298] Allow setting a custom applicatio...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/876
---
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-2298] Allow setting a custom applicatio...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/876#discussion_r33657128
--- Diff: docs/setup/yarn_setup.md ---
@@ -100,11 +100,13 @@ Usage:
-D <arg> Dynamic properties
-d,--detached Start detached
-jm,--jobManagerMemory <arg> Memory for JobManager Container [in MB]
+ -nm,--name Set a custom name for the application on YARN
--- End diff --
This short option `-nm` can be very easily confused with the `-n` parameter which sets the number of containers. I would remove the short option and just have the long one `--name`.
---
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-2298] Allow setting a custom applicatio...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/flink/pull/876#discussion_r33604736
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/FlinkYarnClient.java ---
@@ -591,14 +592,17 @@ protected AbstractFlinkYarnCluster deployInternal(String clusterName) throws Exc
capability.setMemory(jobManagerMemoryMb);
capability.setVirtualCores(1);
- if(clusterName == null) {
- clusterName = "Flink session with "+taskManagerCount+" TaskManagers";
+ if(defaultName == null) {
+ defaultName = "Flink session with "+taskManagerCount+" TaskManagers";
}
if(detached) {
- clusterName += " (detached)";
+ defaultName += " (detached)";
}
- appContext.setApplicationName(clusterName); // application name
+ if(this.customName != null) {
--- End diff --
Should probably check for empty or blank name I suppose
---
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-2298] Allow setting a custom applicatio...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/flink/pull/876#issuecomment-117613418
Thx for the good PR, Robert :)
+1 LGTM
---
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-2298] Allow setting a custom applicatio...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/876#issuecomment-117630964
Looks good except for the test case comment I made.
---
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-2298] Allow setting a custom applicatio...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/876#issuecomment-117663964
:+1: Thanks :)
---
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-2298] Allow setting a custom applicatio...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/876#discussion_r33671148
--- Diff: flink-yarn-tests/src/main/java/org/apache/flink/yarn/YARNSessionFIFOITCase.java ---
@@ -121,6 +121,7 @@ public void testDetachedMode() {
"-n", "1",
"-jm", "768",
"-tm", "1024",
+ "--name", "MyCustomName", // test setting a custom name
--- End diff --
Seems like you only use short options in the tests, so this should be `-nm`.
---
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-2298] Allow setting a custom applicatio...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/876#discussion_r33662338
--- Diff: docs/setup/yarn_setup.md ---
@@ -100,11 +100,13 @@ Usage:
-D <arg> Dynamic properties
-d,--detached Start detached
-jm,--jobManagerMemory <arg> Memory for JobManager Container [in MB]
+ -nm,--name Set a custom name for the application on YARN
--- End diff --
Your concern is justified. But, if somebody is doing `-n mycustomname` it will fail because mycustomname is not numeric ;)
The reason why I have the `-nm` argument as well is because the user who requested the feature suggested to use exactly that name ;)
---
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-2298] Allow setting a custom applicatio...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/flink/pull/876#discussion_r33604696
--- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/FlinkYarnClient.java ---
@@ -341,7 +342,7 @@ public AbstractFlinkYarnCluster run() throws Exception {
* This method will block until the ApplicationMaster/JobManager have been
* deployed on YARN.
*/
- protected AbstractFlinkYarnCluster deployInternal(String clusterName) throws Exception {
+ protected AbstractFlinkYarnCluster deployInternal(String defaultName) throws Exception {
--- End diff --
Since this becomes default, maybe we could just remove the input argument and set default internally if customName is null or empty.
---
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-2298] Allow setting a custom applicatio...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/876#issuecomment-117662372
Which comment? I added a test also for the short variant.
Merging this ...
---
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.
---