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 2018/01/02 08:18:46 UTC
[GitHub] flink pull request #5224: [FLINK-8338] [flip6] Make CustomCommandLines non s...
GitHub user tillrohrmann opened a pull request:
https://github.com/apache/flink/pull/5224
[FLINK-8338] [flip6] Make CustomCommandLines non static in CliFrontend
## What is the purpose of the change
This commit changes how CustomCommandLines are registered at the CliFrontend.
Henceforth, the CliFrontend is initialized with the set of CustomCommandLines
instead of registering them statically. This improves maintainability and
testability.
This PR is based on #5220.
## Brief change log
- Remove static `CustomCommandLines` field in `CliFrontend`
- Pass in the `CustomCommandLines` to the `CliFrontend's` constructor
- Adapt command methods in `CliFrontend` to use the correct `Options` from the instance `CustomCommandLines`
## Verifying this change
- This change is covered by existing tests
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- 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/tillrohrmann/flink nonStaticCustomCommandLines
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5224.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 #5224
----
commit c48324702b0ebff3fd7a16e480ead90fc4c9a30b
Author: Till Rohrmann <tr...@...>
Date: 2017-12-07T12:57:24Z
[FLINK-8328] [flip6] Move Yarn ApplicationStatus polling out of YarnClusterClient
Introduce YarnApplicationStatusMonitor which does the Yarn ApplicationStatus polling in
the FlinkYarnSessionCli. This decouples the YarnClusterClient from the actual communication
with Yarn and, thus, gives a better separation of concerns.
commit 783bd1daf36b34a74cbe718a52a1043ba38d5a44
Author: Till Rohrmann <tr...@...>
Date: 2017-12-20T15:43:21Z
[FLINK-8329] [flip6] Move YarnClient to AbstractYarnClusterDescriptor
Moves the YarnClient from the YarnClusterClient to the AbstractYarnClusterDescriptor.
This makes the latter responsible for the lifecycle management of the client and gives
a better separation of concerns.
commit 192adb786a48d19b71e797355500652d51de6296
Author: Till Rohrmann <tr...@...>
Date: 2017-12-18T17:59:30Z
[FLINK-8332] [flip6] Move savepoint dispose into ClusterClient
Move the savepoint disposal logic from the CliFrontend into the ClusterClient. This gives
a better separation of concerns and allows the CliFrontend to be used with different
ClusterClient implementations.
commit a73b6e2850d4b2445a835914ba570d1057e59dfb
Author: Till Rohrmann <tr...@...>
Date: 2018-01-02T06:42:18Z
[FLINK-8333] [flip6] Separate deployment options from command options
This commit separates the parsing of command options and deployment options into two
steps. This makes it easier to make the CustomCommandLines non-static.
Moreover, this commit moves the CliFrontend into the cli sub package.
commit e32fa9d4eb45bfac2d13ab112f127be29d273ebd
Author: Till Rohrmann <tr...@...>
Date: 2018-01-02T06:59:34Z
[FLINK-8338] [flip6] Make CustomCommandLines non static in CliFrontend
This commit changes how CustomCommandLines are registered at the CliFrontend.
Henceforth, the CliFrontend is initialized with the set of CustomCommandLines
instead of registering them statically. This improves maintainability and
testability.
----
---
[GitHub] flink pull request #5224: [FLINK-8338] [flip6] Make CustomCommandLines non s...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5224#discussion_r159647064
--- Diff: flink-clients/src/test/java/org/apache/flink/client/cli/util/MockedCliFrontend.java ---
@@ -20,40 +20,22 @@
import org.apache.flink.client.cli.CliFrontend;
import org.apache.flink.client.cli.CliFrontendTestUtils;
-import org.apache.flink.client.cli.CustomCommandLine;
import org.apache.flink.client.program.ClusterClient;
import org.apache.flink.configuration.Configuration;
-import org.apache.commons.cli.CommandLine;
-
-import static org.mockito.Matchers.any;
-import static org.mockito.Matchers.anyString;
-import static org.powermock.api.mockito.PowerMockito.mock;
-import static org.powermock.api.mockito.PowerMockito.when;
+import java.util.Collections;
/**
* Utility class for mocking the {@link ClusterClient} within a {@link CliFrontend}.
*
* <p>The mocking behavior can be defined in the constructor of the sub-class.
*/
public class MockedCliFrontend extends CliFrontend {
--- End diff --
nit: Name is not ideal because it's not required that only mocks are passed as a constructor argument.
---
[GitHub] flink pull request #5224: [FLINK-8338] [flip6] Make CustomCommandLines non s...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5224#discussion_r159451928
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontendParser.java ---
@@ -391,35 +353,22 @@ public static void printHelpForSavepoint() {
formatter.setSyntaxPrefix(" \"savepoint\" action options:");
formatter.printHelp(" ", getSavepointOptionsWithoutDeprecatedOptions(new Options()));
- printCustomCliOptions(formatter, false);
+ printCustomCliOptions(customCommandLines, formatter, false);
System.out.println();
}
- /**
- * Adds custom cli options.
- * @param options The options to add options to
- * @param runOptions Whether to include run options
- * @return Options with additions
- */
- private static Options addCustomCliOptions(Options options, boolean runOptions) {
- for (CustomCommandLine cli: CliFrontend.getCustomCommandLineList()) {
- cli.addGeneralOptions(options);
- if (runOptions) {
- cli.addRunOptions(options);
- }
- }
- return options;
- }
-
/**
* Prints custom cli options.
* @param formatter The formatter to use for printing
* @param runOptions True if the run options should be printed, False to print only general options
*/
- private static void printCustomCliOptions(HelpFormatter formatter, boolean runOptions) {
+ private static void printCustomCliOptions(
+ Collection<CustomCommandLine<?>> customCommandLines,
--- End diff --
nit: indentation
---
[GitHub] flink pull request #5224: [FLINK-8338] [flip6] Make CustomCommandLines non s...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5224#discussion_r159643583
--- Diff: flink-clients/src/test/java/org/apache/flink/client/cli/CliFrontendCancelTest.java ---
@@ -133,19 +139,23 @@ public void testCancelWithSavepointWithoutJobId() throws Exception {
public void testCancelWithSavepointWithoutParameters() throws Exception {
// Cancel with savepoint (no target directory) and no job ID
String[] parameters = { "-s" };
- CliFrontend testFrontend = new CliFrontend(CliFrontendTestUtils.getConfigDir());
+ CliFrontend testFrontend = new CliFrontend(
+ new Configuration(),
+ Collections.singletonList(new DefaultCLI()),
+ CliFrontendTestUtils.getConfigDir());
testFrontend.cancel(parameters);
fail("Should have failed.");
}
- private static final class CancelTestCliFrontend extends MockedCliFrontend {
+ private static ClusterClient createClusterClient(boolean reject) throws Exception {
+ final ClusterClient clusterClient = mock(ClusterClient.class);
- CancelTestCliFrontend(boolean reject) throws Exception {
- if (reject) {
- doThrow(new IllegalArgumentException("Test exception")).when(client).cancel(any(JobID.class));
- doThrow(new IllegalArgumentException("Test exception")).when(client).cancelWithSavepoint(any(JobID.class), anyString());
- }
+ if (reject) {
+ doThrow(new IllegalArgumentException("Test exception")).when(clusterClient).cancel(any(JobID.class));
--- End diff --
nit: Code will never run because `reject` is always `false` for all tests. Original author probably forgot to add some test cases.
---
[GitHub] flink pull request #5224: [FLINK-8338] [flip6] Make CustomCommandLines non s...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5224#discussion_r159650780
--- Diff: flink-yarn-tests/src/test/java/org/apache/flink/yarn/CliFrontendYarnAddressConfigurationTest.java ---
@@ -191,27 +213,47 @@ public void testResumeFromYarnID() throws Exception {
@Test
public void testResumeFromYarnIDZookeeperNamespace() throws Exception {
File directoryPath = writeYarnPropertiesFile(validPropertiesFile);
+
// start CLI Frontend
- TestCLI frontend = new CustomYarnTestCLI(directoryPath.getAbsolutePath());
+ TestCLI frontend = new CustomYarnTestCLI(new Configuration(), directoryPath.getAbsolutePath());
- RunOptions options =
- CliFrontendParser.parseRunCommand(new String[] {"-yid", TEST_YARN_APPLICATION_ID.toString()});
+ FlinkYarnSessionCli cli = new FlinkYarnSessionCli("y", "yarn");
- frontend.retrieveClient(options);
+ Options options = CliFrontendParser.getRunCommandOptions();
+ cli.addGeneralOptions(options);
+ cli.addRunOptions(options);
+
+ final CommandLine commandLine = CliFrontendParser.parse(options, new String[] {"-yid", TEST_YARN_APPLICATION_ID.toString()}, true);
+
+ RunOptions runOptions = new RunOptions(commandLine);
+
+ frontend.retrieveClient(runOptions);
String zkNs = frontend.getConfiguration().getValue(HighAvailabilityOptions.HA_CLUSTER_ID);
Assert.assertTrue(zkNs.matches("application_\\d+_0042"));
}
@Test
public void testResumeFromYarnIDZookeeperNamespaceOverride() throws Exception {
File directoryPath = writeYarnPropertiesFile(validPropertiesFile);
+
+ final Configuration configuration = new Configuration();
+ configuration.setString(YarnConfigOptions.PROPERTIES_FILE_LOCATION, directoryPath.getAbsolutePath());
+
// start CLI Frontend
- TestCLI frontend = new CustomYarnTestCLI(directoryPath.getAbsolutePath());
+ TestCLI frontend = new CustomYarnTestCLI(configuration, directoryPath.getAbsolutePath());
String overrideZkNamespace = "my_cluster";
- RunOptions options =
- CliFrontendParser.parseRunCommand(new String[] {"-yid", TEST_YARN_APPLICATION_ID.toString(), "-yz", overrideZkNamespace});
- frontend.retrieveClient(options);
+ FlinkYarnSessionCli cli = new FlinkYarnSessionCli("y", "yarn");
--- End diff --
There is redundancy in how `FlinkYarnSessionCli` and `Options` are created. Maybe it makes sense to extract something to instance variables.
---
[GitHub] flink pull request #5224: [FLINK-8338] [flip6] Make CustomCommandLines non s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/5224
---
[GitHub] flink pull request #5224: [FLINK-8338] [flip6] Make CustomCommandLines non s...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5224#discussion_r159640707
--- Diff: flink-clients/src/main/java/org/apache/flink/client/cli/CliFrontendParser.java ---
@@ -448,68 +397,31 @@ public static RunOptions parseRunCommand(String[] args) throws CliArgsException
}
}
- public static ListOptions parseListCommand(String[] args) throws CliArgsException {
- try {
- DefaultParser parser = new DefaultParser();
- CommandLine line = parser.parse(LIST_OPTIONS, args, false);
- return new ListOptions(line);
- }
- catch (ParseException e) {
- throw new CliArgsException(e.getMessage());
- }
- }
-
- public static CancelOptions parseCancelCommand(String[] args) throws CliArgsException {
- try {
- DefaultParser parser = new DefaultParser();
- CommandLine line = parser.parse(CANCEL_OPTIONS, args, false);
- return new CancelOptions(line);
- }
- catch (ParseException e) {
- throw new CliArgsException(e.getMessage());
- }
- }
+ public static CommandLine parse(Options options, String[] args, boolean stopAtNonOptions) throws CliArgsException {
+ final DefaultParser parser = new DefaultParser();
- public static StopOptions parseStopCommand(String[] args) throws CliArgsException {
try {
- DefaultParser parser = new DefaultParser();
- CommandLine line = parser.parse(STOP_OPTIONS, args, false);
- return new StopOptions(line);
+ return parser.parse(options, args, stopAtNonOptions);
} catch (ParseException e) {
throw new CliArgsException(e.getMessage());
}
}
- public static SavepointOptions parseSavepointCommand(String[] args) throws CliArgsException {
- try {
- DefaultParser parser = new DefaultParser();
- CommandLine line = parser.parse(SAVEPOINT_OPTIONS, args, false);
- return new SavepointOptions(line);
- }
- catch (ParseException e) {
- throw new CliArgsException(e.getMessage());
- }
- }
+ static Options mergeOptions(Options optionsA, Options optionsB) {
--- End diff --
nit: Missing `@Nullable` on parameters & Javadoc
---
[GitHub] flink pull request #5224: [FLINK-8338] [flip6] Make CustomCommandLines non s...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5224#discussion_r159644962
--- Diff: flink-clients/src/test/java/org/apache/flink/client/cli/CliFrontendPackageProgramTest.java ---
@@ -60,7 +62,10 @@ public static void init() {
@Test
public void testNonExistingJarFile() {
try {
- CliFrontend frontend = new CliFrontend(CliFrontendTestUtils.getConfigDir());
+ CliFrontend frontend = new CliFrontend(
--- End diff --
Just an observation: The diff is only big because the instance under test is not an instance variable, and set up in a `@Before` annotated method.
---
[GitHub] flink pull request #5224: [FLINK-8338] [flip6] Make CustomCommandLines non s...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5224#discussion_r159641680
--- Diff: flink-clients/src/test/java/org/apache/flink/client/cli/CliFrontendAddressConfigurationTest.java ---
@@ -47,36 +49,29 @@ public static void init() {
}
@Test
- public void testValidConfig() {
- try {
- CliFrontend frontend = new CliFrontend(CliFrontendTestUtils.getConfigDir());
- RunOptions options = CliFrontendParser.parseRunCommand(new String[] {});
-
- ClusterClient clusterClient = frontend.retrieveClient(options);
+ public void testValidConfig() throws Exception {
+ CliFrontend frontend = new CliFrontend(
+ GlobalConfiguration.loadConfiguration(CliFrontendTestUtils.getConfigDir()),
+ Collections.singletonList(new DefaultCLI()),
+ CliFrontendTestUtils.getConfigDir());
- checkJobManagerAddress(
- clusterClient.getFlinkConfiguration(),
- CliFrontendTestUtils.TEST_JOB_MANAGER_ADDRESS,
- CliFrontendTestUtils.TEST_JOB_MANAGER_PORT);
- }
- catch (Exception e) {
- e.printStackTrace();
- fail(e.getMessage());
- }
- }
-
- @Test(expected = IllegalConfigurationException.class)
- public void testInvalidConfigAndNoOption() throws Exception {
- CliFrontend frontend = new CliFrontend(CliFrontendTestUtils.getInvalidConfigDir());
RunOptions options = CliFrontendParser.parseRunCommand(new String[] {});
- frontend.retrieveClient(options);
+ ClusterClient clusterClient = frontend.retrieveClient(options);
+
+ checkJobManagerAddress(
+ clusterClient.getFlinkConfiguration(),
+ CliFrontendTestUtils.TEST_JOB_MANAGER_ADDRESS,
+ CliFrontendTestUtils.TEST_JOB_MANAGER_PORT);
}
@Test
public void testManualOptionsOverridesConfig() {
try {
- CliFrontend frontend = new CliFrontend(CliFrontendTestUtils.getConfigDir());
+ CliFrontend frontend = new CliFrontend(
--- End diff --
nit: try-catch-exception block was removed in the test above but not here.
---
[GitHub] flink issue #5224: [FLINK-8338] [flip6] Make CustomCommandLines non static i...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/5224
Thanks for your review @GJL. I've addressed your comments and rebased onto the latest master.
---
[GitHub] flink pull request #5224: [FLINK-8338] [flip6] Make CustomCommandLines non s...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/5224#discussion_r160946952
--- Diff: flink-clients/src/test/java/org/apache/flink/client/cli/util/MockedCliFrontend.java ---
@@ -20,40 +20,22 @@
import org.apache.flink.client.cli.CliFrontend;
import org.apache.flink.client.cli.CliFrontendTestUtils;
-import org.apache.flink.client.cli.CustomCommandLine;
import org.apache.flink.client.program.ClusterClient;
import org.apache.flink.configuration.Configuration;
-import org.apache.commons.cli.CommandLine;
-
-import static org.mockito.Matchers.any;
-import static org.mockito.Matchers.anyString;
-import static org.powermock.api.mockito.PowerMockito.mock;
-import static org.powermock.api.mockito.PowerMockito.when;
+import java.util.Collections;
/**
* Utility class for mocking the {@link ClusterClient} within a {@link CliFrontend}.
*
* <p>The mocking behavior can be defined in the constructor of the sub-class.
*/
public class MockedCliFrontend extends CliFrontend {
--- End diff --
True. But since I didn't introduce it, I'll keep it like it is (avoiding further downstream merge conflicts).
---