[FLINK-7791] [REST][client] Integrate LIST command into RestClusterClient #4802

zentol · 2017-10-11T13:06:34Z

What is the purpose of the change

This PR integrates the LIST command into the RestClusterClient. This is a fully working implementation and leverages the CurrentJobsOverviewHeaders.

Brief change log

move LIST logic from CliFrontend into ClusterClient
separate CliFrontendListCancelTest into distinct test cases for list & cancel
implement LIST logic for RestClusterClient, based on the CurrentJobsOverviewHeaders

Verifying this change

This change added tests and can be verified as follows:

the changes to the CliFrontend are covered by modified tests in CliFrontendListTest
the changes to the ClusterClient are covered by new tests in ClusterClientTest
the changes to the RestClusterClient are covered by RestClusterClientTest#testListJobs

For manual verification:

start a flip6 cluster with start-cluster.sh flip6
submit a long-running job with flink run -flip <jar>
execute the list command with flink list -flip6

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: (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not documented)

…te argument

…ient

tillrohrmann

LGTM.

The only thing I was wondering about was whether the ClusterClient should return a Collection<JobStatus> instead of a CompletableFuture<Collection<JobStatus>>. I would assume that the user would want this operation to be executed synchronously, but I'm not entirely sure.

tillrohrmann · 2017-10-11T13:08:49Z

flink-clients/src/main/java/org/apache/flink/client/CliFrontend.java

-			}
-			catch (Exception e) {
-				throw new Exception("Could not retrieve running jobs from the JobManager.", e);
+				CompletableFuture<Collection<JobDetails>> jobDetailsFuture = client.listJobs();


Should we hide the fact that we are retrieving JobDetails internally and still only return a Collection<JobStatus>? The reason is that in the future we might change how we retrieve the job ids, job names plus their state.

Even if we change how we retrieve it we can still wrap them in a JobDetails object; it doesn't necessarily leak to the outside how we retrieve them. Given that the CLI requires the job id, job name, start/end timestamp and job status we aren't saving much by creating a slimmed down JobDetails class.

I thought about the JobStatusMessage class. I'm not entirely sure, because whatever you expose can be used and once we change that we no longer retrieve the full set of details for a job, it will be difficult to change it.

I can change it to that.

tillrohrmann · 2017-10-11T13:10:26Z

flink-clients/src/main/java/org/apache/flink/client/program/rest/RestClusterClient.java

+			headers
+		);
+		return jobDetailsFuture
+			.thenApply(MultipleJobsDetails::getRunning);


I think we also have to return the list of completed jobs not only the running.

we don't have to, as the LIST command only cares about running jobs, but we certainly could just return all. The CLI is filtering the jobs anyway.

tillrohrmann · 2017-10-11T13:10:49Z

flink-clients/src/test/java/org/apache/flink/client/CliFrontendListTest.java

+import static org.powermock.api.mockito.PowerMockito.when;
+
+/**
+ * Tests for the CANCEL command.


typo: LIST command.

tillrohrmann · 2017-10-11T13:15:43Z

flink-clients/src/test/java/org/apache/flink/client/program/ClusterClientTest.java

+			Collection<JobDetails> jobDetails = jobDetailsFuture.get();
+			Assert.assertTrue(gateway.messageArrived);
+			// finished jobs should be ignored
+			Assert.assertEquals(1, jobDetails.size());


Shouldn't we also retrieve the finished jobs here?

~~No, the LIST command only lists running/scheduled jobs.~~

tillrohrmann · 2017-10-11T13:19:17Z

flink-clients/src/main/java/org/apache/flink/client/program/ClusterClient.java

+	 *
+	 * @return future collection of running jobs
+	 * @throws Exception if  no connection to the cluster could be established
+	 */


I'm wondering whether the ClusterClient should issue a synchronous operation instead of a asynchronous operation (simply waiting on the futures completion). So far all the ClusterClient operations are blocking and wait for the completion. Not entirely sure which is more intuitive to use for the user. Of course the asynchronous variant gives more freedom.

zentol · 2017-10-11T13:25:19Z

The reason why i went for the CompletableFuture route is to stay closer to the current code. The CliFrontend is waiting the the response and prints "Waiting...", which we can't do when doing the call synchronously since the ClusterClient shouldn't print anything.

That said, i do like making them asynchronous in general.

zentol · 2017-10-11T13:51:21Z

I've modified the clients to return both running and finished jobs.

tillrohrmann · 2017-10-11T14:03:44Z

I'm not entirely sure whether printing "Waiting..." is consistent with the status quo. The current list implementation does not do it. Moreover, so far all ClusterClient operations are synchronous (e.g. stop and cancel). In general I don't have strong feelings about returning a future or the result, but so far I think we always made the call synchronous and, thus, diverge from it with this change.

zentol · 2017-10-11T14:25:15Z

triggerSavepoint is also asynchronous. It is true that the list command does not print waiting, but the savepoint commands do.

tillrohrmann · 2017-10-11T15:58:00Z

But triggerSavepoint is not a command of the ClusterClient but of the CliFrontend. All other ClusterClient commands like stop and cancel behave differently.

zentol · 2017-10-11T17:43:52Z

What do you mean when you say triggerSavepoint is not a command of the ClusterClient? It does have a triggerSavepoint method.

tillrohrmann · 2017-10-12T13:46:28Z

True, you're right. I guess I overlooked it. But it also includes the trigger prefix which indicates that it does not take a savepoint but only triggers it. If we want to be consistent, then we would also return a future for the stop and cancel call on which you can wait if you want to wait for the completion of the call. But I guess that this part will be reworked anyway in the future when we refactor the client. Thus, I guess you can merge the PR.

zentol · 2017-10-15T15:15:59Z

merging.

zentol added 4 commits October 11, 2017 13:55

[FLINK-7808] [REST] JobDetails constructor checks size of tasksPerSta…

cc80c68

…te argument

[FLINK-7807] [REST] Log exceptions in HandlerUtils methods

4463158

[FLINK-7791] [Client] Move LIST logic into ClusterClient

274c343

[FLINK-7791] [REST][client] Integrate LIST command into RestClusterCl…

eb6beff

…ient

tillrohrmann approved these changes Oct 11, 2017

View reviewed changes

zentol added 4 commits October 11, 2017 15:44

rename CliFrontendListCancelTest

4694b8f

Fix typo in CliFrontendListTest javaodc

700e2a5

ClusterClient#listJObs returns both running and finished jobs

d48a4b7

RestClusterClient#listJObs returns both running and finished jobs

83f61e4

zentol added 2 commits October 11, 2017 16:35

modify ClusterClient to return JobStatusMessage

a6526bf

Modify RestClusterClient to return JobStatusMessage

b88aacb

zentol added 3 commits October 12, 2017 15:58

checkstyle

b2f876d

checkstyle

9e857ea

checkstyle

948bfad

asfgit closed this in a76de28 Oct 15, 2017

zentol deleted the 7791 branch October 15, 2017 17:01

rmetzger added component=CommandLineClient component=Runtime/REST labels Mar 18, 2019

[FLINK-7791] [REST][client] Integrate LIST command into RestClusterClient #4802

[FLINK-7791] [REST][client] Integrate LIST command into RestClusterClient #4802

Uh oh!

Conversation

zentol commented Oct 11, 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

Uh oh!

tillrohrmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zentol Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zentol commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zentol commented Oct 11, 2017

Uh oh!

tillrohrmann commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zentol commented Oct 11, 2017

Uh oh!

tillrohrmann commented Oct 11, 2017

Uh oh!

zentol commented Oct 11, 2017

Uh oh!

tillrohrmann commented Oct 12, 2017

Uh oh!

zentol commented Oct 15, 2017

Uh oh!

Uh oh!

zentol Oct 11, 2017 •

edited

Loading

zentol commented Oct 11, 2017 •

edited

Loading

tillrohrmann commented Oct 11, 2017 •

edited

Loading