[FLINK-8328] [flip6] Move Yarn ApplicationStatus polling out of YarnClusterClient #5215

tillrohrmann · 2017-12-29T17:17:31Z

What is the purpose of the change

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.

Brief change log

Replace the PollingThread with the YarnApplicationStatusMonitor
Decouple YarnClusterClient from Yarn ApplicationStatus polling

Verifying this change

Changes 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)

GJL

In general it looks good. I left some comments.

GJL · 2018-01-02T13:22:17Z

flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java

+
+	private static void printClusterMessages(YarnClusterClient clusterClient) {
+		final List<String> messages = clusterClient.getNewMessages();
+		if (messages != null && messages.size() > 0) {


nit: if (!messages.isEmpty()) should suffice because messages is never null

true. Will change it.

GJL · 2018-01-02T13:29:46Z

flink-yarn/src/main/java/org/apache/flink/yarn/cli/YarnApplicationStatusMonitor.java

+
+	@Override
+	public void close() throws Exception {
+		applicationStatusUpdateFuture.cancel(false);


There is no need to declare throws Exception here because cancel() does not throw any checked exceptions.

true, will remove it.

GJL · 2018-01-02T13:32:23Z

flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java

+						yarnCluster,
+						yarnApplicationStatusMonitor,
+						true);
+				} catch (Exception e) {


Closing YarnApplicationStatusMonitor should not throw any checked exceptions. If you change the signature, this catch block won't be needed.

True, will remove it.

GJL · 2018-01-02T13:39:54Z

flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java

+				try (YarnApplicationStatusMonitor yarnApplicationStatusMonitor = new YarnApplicationStatusMonitor(
+						yarnDescriptor.getYarnClient(),
+						yarnCluster.getApplicationId(),
+						new ScheduledExecutorServiceAdapter(scheduledExecutorService))) {


Why do we need to use the ScheduledExecutor interface from Flink? Why not use Java's ScheduledExecutorService directly?

The ScheduledExecutor gives a better abstraction because it does not expose service control methods like shutdown to the callee. I think the Java abstraction is slightly broken in this regard.

GJL · 2018-01-02T13:41:02Z

flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java

+						yarnApplicationStatusMonitor,
+						acceptInteractiveInput);
+				} catch (Exception e) {
+					LOG.info("Could not properly close the Yarn application status monitor.", e);


Same here. Catch block could be avoided.

Changed it.

GJL · 2018-01-02T13:43:21Z

flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java

@@ -660,7 +570,25 @@ public int run(
 					"yarn application -kill " + applicationId.getOpt());
 				yarnCluster.disconnect();
 			} else {
-				runInteractiveCli(yarnCluster, true);
+				ScheduledThreadPoolExecutor scheduledExecutorService = new ScheduledThreadPoolExecutor(1);


I think the executor could as well be in the Monitor. If needed in the future, one could provide a constructor that accepts an external executor (e.g., for unit tests).

Yes it could be. That way, however, we support that we can use an arbitrary executor which is available (as you've mentioned for tests). Since refactoring wouldn't add much value, I'll keep it like this.

GJL · 2018-01-02T13:45:23Z

flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java

+					runInteractiveCli(
+						yarnCluster,
+						yarnApplicationStatusMonitor,
+						acceptInteractiveInput);


The code block looks duplicated except for this flag.

Yes it is. In one of my later PRs, I removed this code duplication. Therefore I leave it like this for the moment.

GJL · 2018-01-02T14:00:48Z

flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java

+		try (BufferedReader in = new BufferedReader(new InputStreamReader(System.in))) {
+			boolean continueRepl = true;
+			int numTaskmanagers = 0;
+			long unknownStatusSince = System.currentTimeMillis();


nit: System.nanoTime() should be preferred to measure elapsed time because it does not depend on wall clock, i.e., it is not affected by the user changing the system's time: https://stackoverflow.com/a/351571
However, if you use nanoTime(), the trick in line 729 with negative unknownStatusSince won't work.

tillrohrmann · 2018-01-10T13:49:25Z

Thanks for the review @GJL. I've addressed your comments. Once Travis gives green light, I'll merge the PR.

…lusterClient 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.

…f YarnClusterClient

tillrohrmann mentioned this pull request Dec 29, 2017

[FLINK-8329] [flip6] Move YarnClient to AbstractYarnClusterDescriptor #5216

Closed

tillrohrmann force-pushed the removeSpecialClusterClients branch from 69ef978 to c483247 Compare December 31, 2017 17:30

GJL reviewed Jan 2, 2018

View reviewed changes

tillrohrmann force-pushed the removeSpecialClusterClients branch 2 times, most recently from daf7536 to d7f4d2c Compare January 10, 2018 14:22

tillrohrmann added 3 commits January 11, 2018 13:13

fixup! [FLINK-8328] [flip6] Move Yarn ApplicationStatus polling out o…

368a94f

…f YarnClusterClient

fixup! [FLINK-8328] [flip6] Move Yarn ApplicationStatus polling out o…

53d0103

…f YarnClusterClient

tillrohrmann force-pushed the removeSpecialClusterClients branch from d7f4d2c to 53d0103 Compare January 11, 2018 12:13

asfgit closed this in 2ce5b98 Jan 11, 2018

tillrohrmann deleted the removeSpecialClusterClients branch January 11, 2018 16:20

rmetzger added the component=CommandLineClient label Mar 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-8328] [flip6] Move Yarn ApplicationStatus polling out of YarnClusterClient #5215

[FLINK-8328] [flip6] Move Yarn ApplicationStatus polling out of YarnClusterClient #5215

tillrohrmann commented Dec 29, 2017

GJL left a comment

GJL Jan 2, 2018

tillrohrmann Jan 10, 2018

GJL Jan 2, 2018

tillrohrmann Jan 10, 2018

GJL Jan 2, 2018

tillrohrmann Jan 10, 2018

GJL Jan 2, 2018

tillrohrmann Jan 10, 2018

GJL Jan 2, 2018

tillrohrmann Jan 10, 2018

GJL Jan 2, 2018

tillrohrmann Jan 10, 2018

GJL Jan 10, 2018

GJL Jan 2, 2018

tillrohrmann Jan 10, 2018

GJL Jan 10, 2018

GJL Jan 2, 2018

tillrohrmann commented Jan 10, 2018

[FLINK-8328] [flip6] Move Yarn ApplicationStatus polling out of YarnClusterClient #5215

[FLINK-8328] [flip6] Move Yarn ApplicationStatus polling out of YarnClusterClient #5215

Conversation

tillrohrmann commented Dec 29, 2017

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

GJL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann commented Jan 10, 2018