You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by NicoK <gi...@git.apache.org> on 2018/03/09 11:25:49 UTC
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
GitHub user NicoK opened a pull request:
https://github.com/apache/flink/pull/5672
[FLINK-8872][flip6] fix yarn detached mode command parsing
## What is the purpose of the change
Running yarn per-job cluster in detached mode via `-yd` currently does not detach and instead waits for the job to finish ( parameter `-d` works, through).
Example:
```
./bin/flink run -m yarn-cluster -yn 10 -yjm 768 -ytm 3072 -ys 2 -yd -p 20 -c org.apache.flink.streaming.examples.wordcount.WordCount ./examples/streaming/WordCount.jar --input <file>
```
The output somewhat says it is running detached but it will not:
```
2018-03-05 13:41:28,343 INFO org.apache.flink.yarn.AbstractYarnClusterDescriptor - The Flink YARN client has been started in detached mode. In order to stop Flink on YARN, use the following command or a YARN web interface to stop it:
yarn application -kill application_1519984124671_0006
Please also note that the temporary files of the YARN session in the home directoy will not be removed.
Starting execution of program
```
Please note that this PR also includes #5671 and #5670.
## Brief change log
- move `isDetachedMode()` switch from `ProgramOptions` to `CustomCommandLine`
- extend some job submission debugging messages with detached mode info
for the tests:
- create test-jar in `flink-clients`
- add test-scope dependency from `flink-yarn` to the test-jar of `flink-clients`
- add a small job jar file (with the word count from `flink-clients`) for deployment tests to `flink-yarn`
## Verifying this change
This change added tests and can be verified as follows:
- added verification of `CustomCommandLine#isDetachedMode()` to `CliFrontendRunTest`
- add `FlinkYarnSessionCliTest#testCorrectSettingOfDetachedMode()`
- add `CliFrontendRunTestWithYarn` test which tests `CliFrontend` together with `FlinkYarnSessionCli` (this tests what was previously not working - each individual component was tested but the error did not show in those tests)
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): **only internally**
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: **no**
- The serializers: **no**
- The runtime per-record code paths (performance sensitive): **no**
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: **yes**
- The S3 file system connector: **no**
## Documentation
- Does this pull request introduce a new feature? **no**
- If yes, how is the feature documented? **not applicable**
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/NicoK/flink flink-8872
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5672.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 #5672
----
commit 77cea62307c49cddc9b0699b36dfcb81d52a2f44
Author: Nico Kruber <ni...@...>
Date: 2018-03-09T09:56:31Z
[hotfix][cli][tests] let CliFrontendRunTest extend from TestLogger
commit 49a502f50abf9d351ac0adbb1a78bfbc9f06cd73
Author: Nico Kruber <ni...@...>
Date: 2018-03-06T10:43:32Z
[FLINK-8904][cli][tests] always restore the previous sysout when changing it in the test
commit 417689deed94b00228e3959ba7ccdfc71985d8ac
Author: Nico Kruber <ni...@...>
Date: 2018-03-09T10:05:51Z
[FLINK-8905][rest][client] fix RestClusterClient#getMaxSlots() returning 0
commit 3f2c31e7959038f0699cfcdc157ccc3b15a39ee1
Author: Nico Kruber <ni...@...>
Date: 2018-03-05T17:24:17Z
[FLINK-8872][flip6] fix yarn detached mode command parsing
The detached flag if given by "-yd" was not passed correctly into the
CliFrontend and resulted in the CLI waiting for submitted jobs to finish instead
of detaching from the execution.
commit 47a92c5c34c7422f809f5e2424221c8bdb2b8cc8
Author: Nico Kruber <ni...@...>
Date: 2018-03-08T10:07:08Z
[FLINK-8906][flip6][tests] also test Flip6DefaultCLI in org.apache.flink.client.cli tests
commit e4bc32b4960242ea5c3ec762e76fa40d7eeb540b
Author: Nico Kruber <ni...@...>
Date: 2018-03-08T11:07:27Z
[FLINK-8872][yarn] add tests for YARN detached mode command line parsing with CliFrontend
- create a test-jar of flink-clients
- create a jar with test programs inside flink-yarn (copy from flink-clients)
- create CliFrontendRunTestWithYarn based on CliFrontendRunTest that verifies
CliFrontend's parsing in conjunction with FlinkYarnSessionCli
-> verify detached mode in this test (can be extended further in the future)
commit 37cf32b42b451e90d4658e203889ae2542ad3a59
Author: Nico Kruber <ni...@...>
Date: 2018-03-09T11:11:55Z
[hotfix][typo] fix typo in AbstractYarnClusterDescriptor
----
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r175713114
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontend.java ---
@@ -225,7 +226,9 @@ protected void run(String[] args) throws Exception {
final ClusterClient<T> client;
// directly deploy the job if the cluster is started in job mode and detached
- if (flip6 && clusterId == null && runOptions.getDetachedMode()) {
+ boolean detachedMode = customCommandLine.isDetachedMode(commandLine);
+ LOG.debug("Detached job mode is set to {}", detachedMode);
+ if (flip6 && clusterId == null && detachedMode) {
--- End diff --
The `RunOptions` should already contain the information whether we are in detached mode or not.
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r175713202
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CustomCommandLine.java ---
@@ -97,4 +97,12 @@ default CommandLine parseCommandLineOptions(String[] args, boolean stopAtNonOpti
addRunOptions(options);
return CliFrontendParser.parse(options, args, stopAtNonOptions);
}
+
+ /**
+ * Returns whether the call should be detached or not.
+ *
+ * @param commandLine command line containing options relevant for the detached mode retrieval
+ * @return <tt>true</tt> if it should run detached, <tt>false</tt> otherwise
+ */
+ boolean isDetachedMode(CommandLine commandLine);
--- End diff --
See comment above.
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r175712900
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/AbstractCustomCommandLine.java ---
@@ -91,4 +91,9 @@ protected Configuration applyCommandLineOptionsToConfiguration(CommandLine comma
return resultingConfiguration;
}
+
+ @Override
+ public boolean isDetachedMode(CommandLine commandLine) {
--- End diff --
I think the `CustomCommandLine` should not have this method. It is more of a factory for a `ClusterDescriptor` and general options like this should go to the `CLI` class.
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r175756295
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontend.java ---
@@ -225,7 +226,9 @@ protected void run(String[] args) throws Exception {
final ClusterClient<T> client;
// directly deploy the job if the cluster is started in job mode and detached
- if (flip6 && clusterId == null && runOptions.getDetachedMode()) {
+ boolean detachedMode = customCommandLine.isDetachedMode(commandLine);
+ LOG.debug("Detached job mode is set to {}", detachedMode);
+ if (flip6 && clusterId == null && detachedMode) {
--- End diff --
I would be willing to do so. Alternatively, we could register a `-yd` option in `CliFrontend` for backwards compatibility reasons. This would, however, also have an effect if used with standalone mode.
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/5672
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r175746873
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontend.java ---
@@ -225,7 +226,9 @@ protected void run(String[] args) throws Exception {
final ClusterClient<T> client;
// directly deploy the job if the cluster is started in job mode and detached
- if (flip6 && clusterId == null && runOptions.getDetachedMode()) {
+ boolean detachedMode = customCommandLine.isDetachedMode(commandLine);
+ LOG.debug("Detached job mode is set to {}", detachedMode);
+ if (flip6 && clusterId == null && detachedMode) {
--- End diff --
Yes, I think there should not be any `-yd`.
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r175714576
--- Diff: flink-yarn/src/test/java/org/apache/flink/yarn/CliFrontendRunTestWithYarn.java ---
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.yarn;
+
+import org.apache.flink.client.cli.CliFrontendTestUtils;
+import org.apache.flink.client.deployment.ClusterSpecification;
+import org.apache.flink.client.program.ClusterClient;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.configuration.CoreOptions;
+import org.apache.flink.configuration.JobManagerOptions;
+import org.apache.flink.runtime.jobgraph.JobGraph;
+import org.apache.flink.util.FlinkException;
+import org.apache.flink.yarn.cli.FlinkYarnSessionCli;
+import org.apache.flink.yarn.util.FakeClusterClient;
+import org.apache.flink.yarn.util.NonDeployingYarnClusterDescriptor;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.hadoop.yarn.api.records.ApplicationId;
+import org.apache.hadoop.yarn.client.api.YarnClient;
+import org.apache.hadoop.yarn.conf.YarnConfiguration;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static junit.framework.TestCase.assertTrue;
+import static org.apache.flink.client.cli.CliFrontendRunTest.verifyCliFrontend;
+
+/**
+ * Tests for the RUN command using a {@link org.apache.flink.yarn.cli.FlinkYarnSessionCli} inside
+ * the {@link org.apache.flink.client.cli.CliFrontend}.
+ *
+ * @see org.apache.flink.client.cli.CliFrontendRunTest
+ */
+@RunWith(Parameterized.class)
+public class CliFrontendRunTestWithYarn {
--- End diff --
`extends TestLogger` is missing
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r175752444
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontend.java ---
@@ -225,7 +226,9 @@ protected void run(String[] args) throws Exception {
final ClusterClient<T> client;
// directly deploy the job if the cluster is started in job mode and detached
- if (flip6 && clusterId == null && runOptions.getDetachedMode()) {
+ boolean detachedMode = customCommandLine.isDetachedMode(commandLine);
+ LOG.debug("Detached job mode is set to {}", detachedMode);
+ if (flip6 && clusterId == null && detachedMode) {
--- End diff --
I agree that we shouldn't have `-yd`, but it is part of the client API and unless we want to change existing behavior we can't rely on `RunOptions` alone.
---
[GitHub] flink issue #5672: [FLINK-8872][flip6] fix yarn detached mode command parsin...
Posted by NicoK <gi...@git.apache.org>.
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/5672
I reworked the code as desired, i.e. adding `-yd` and `--yarndetached` as deprecated parameters to `CliFrontendParser` and adapting `ProgramOptions` and `FlinkYarnSessionCli` accordingly.
---
[GitHub] flink issue #5672: [FLINK-8872][flip6] fix yarn detached mode command parsin...
Posted by NicoK <gi...@git.apache.org>.
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/5672
ok, after fixing one typo, this should pass Travis now
---
[GitHub] flink issue #5672: [FLINK-8872][flip6] fix yarn detached mode command parsin...
Posted by NicoK <gi...@git.apache.org>.
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/5672
thanks for the review, I also did not like the side-effect approach and after thinking a bit about your first message, I independently came up with the same thing as you proposed in the second one :p
-> rebased onto latest #5671 and added a fixup commit with that change
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r175713468
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/ProgramOptions.java ---
@@ -56,8 +55,6 @@
private final boolean stdoutLogging;
- private final boolean detachedMode;
--- End diff --
I think we should not remove these fields. The `ProgramOptions` represent general options for running a program. The execution behaviour is in my opinion one of these options.
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by NicoK <gi...@git.apache.org>.
Github user NicoK commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r178804566
--- Diff: flink-yarn/pom.xml ---
@@ -326,6 +334,62 @@ under the License.
</execution>
</executions>
</plugin>
+
+ <!-- Test-jar for unit tests, more information on this:
+ http://stackoverflow.com/questions/1401857/using-maven-to-build-separate-jar-files-for-unit-testing-a-custom-class-loader
+ -->
+ <plugin>
+ <artifactId>maven-assembly-plugin</artifactId>
--- End diff --
good idea
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r175724663
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontend.java ---
@@ -225,7 +226,9 @@ protected void run(String[] args) throws Exception {
final ClusterClient<T> client;
// directly deploy the job if the cluster is started in job mode and detached
- if (flip6 && clusterId == null && runOptions.getDetachedMode()) {
+ boolean detachedMode = customCommandLine.isDetachedMode(commandLine);
+ LOG.debug("Detached job mode is set to {}", detachedMode);
+ if (flip6 && clusterId == null && detachedMode) {
--- End diff --
not in the case of yarn. `RunOptions` only evaluates `-d`, not `-yd`.
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r175721144
--- Diff: flink-yarn/pom.xml ---
@@ -326,6 +334,62 @@ under the License.
</execution>
</executions>
</plugin>
+
+ <!-- Test-jar for unit tests, more information on this:
+ http://stackoverflow.com/questions/1401857/using-maven-to-build-separate-jar-files-for-unit-testing-a-custom-class-loader
+ -->
+ <plugin>
+ <artifactId>maven-assembly-plugin</artifactId>
--- End diff --
Are we doing this only to create a `maven-test.jar` in the `flink-yarn` module? Maybe we could move the respective test to `flink-yarn-tests` where we already create some user code jars.
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r174161228
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/Flip6DefaultCLI.java ---
@@ -40,6 +40,7 @@ public Flip6DefaultCLI(Configuration configuration) {
@Override
public boolean isActive(CommandLine commandLine) {
+ this.detachedMode = commandLine.hasOption(CliFrontendParser.DETACHED_OPTION.getOpt());
--- End diff --
I wonder if there is no better place to initialize this field, instead of doing it as undocumented side-effect of something that sounds like an unrelated getter?
---
[GitHub] flink pull request #5672: [FLINK-8872][flip6] fix yarn detached mode command...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/5672#discussion_r174194372
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/Flip6DefaultCLI.java ---
@@ -40,6 +40,7 @@ public Flip6DefaultCLI(Configuration configuration) {
@Override
public boolean isActive(CommandLine commandLine) {
+ this.detachedMode = commandLine.hasOption(CliFrontendParser.DETACHED_OPTION.getOpt());
--- End diff --
Why not introduce an abstract method `isDetached(CommandLine)` in `CustomCommandLine`?
---