[FLINK-13946] Remove job session related code from ExecutionEnvironment #9607

kl0u · 2019-09-04T07:52:16Z

What is the purpose of the change

This PR removes code related to JobSessions from the ExecutionEnvironment and the PlanExecutors. This code was added in the context of FLINK-2097 but it was never activated, as illustrated by the comment at ExecutionEnvironment.java#L285 . The work in this PR is part of the preparation for the upcoming re-design of the whole Client/Executor API.

Brief change log

The changes in the subclasses of the ExecutionEnvironment remove methods that were setting session-related parameters and reflect the simplification of the PlanExecutor lifecycle explained below (for Local and RemoteEnvironment).

The changes to the PlanExecutors have to do with the executor's lifecycle. Now the executor itself controls its lifecycle (start() and stop() are private) and we instantiate an executor for each call to executePlan(). This allows to get rid of the reapers from the Local and RemoteEnvironments and the lock that protected concurrent access to the executor's state.

The lifecycle is more explicit now and aligned with the current use of the ExecutionEnvironment. If in the future we choose to change this and decide to re-use execution environments, then we can add this functionality back, potentially under a different design/architecture.

Verifying this change

This change is a code cleanup so it is covered by existing tests.

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

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

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

Please have a look at this one @aljoscha and @tillrohrmann.

flinkbot · 2019-09-04T07:55:14Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 5267206 (Fri Sep 06 13:30:35 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2019-09-04T08:02:39Z

CI report:

0c2c5cd : SUCCESS Build
9c152ea : FAILURE Build
833e15c : CANCELED Build
5d0397b : CANCELED Build
d256fa7 : FAILURE Build
3814699 : FAILURE Build
8849197 : CANCELED Build
5267206 : SUCCESS Build

tisonkun

Thanks @kl0u for opening this pull requests. I left some inline comments.

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

tisonkun · 2019-09-04T09:49:29Z

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

-				}
+		ClusterClient<?>  client = null;
+		try {
+			client = startClusterClient();


inline startClusterClient and stopClusterClient make more sense to me.

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

flink-java/src/main/java/org/apache/flink/api/java/ExecutionEnvironment.java

flink-java/src/main/java/org/apache/flink/api/java/LocalEnvironment.java

kl0u · 2019-09-04T12:10:27Z

Hi @tisonkun ! Thanks for the review. I integrated your comments. Please have a look and let me know what you think.

aljoscha

This looks good! But there is still some superfluous/needlessly complicated code, IMO.

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

aljoscha · 2019-09-04T13:57:22Z

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

+	private Configuration createExecutorServiceConfig() {
+		final Configuration newConfiguration = new Configuration();
+		newConfiguration.setInteger(TaskManagerOptions.NUM_TASK_SLOTS, taskManagerNumSlots);
+		newConfiguration.setBoolean(CoreOptions.FILESYTEM_DEFAULT_OVERRIDE, defaultOverwriteFiles);


I only see usage of this setter in one single test, maybe we can also remove that or at least not use a field for this anymore.

You mean the setTaskManagerNumSlots() setter?

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

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

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

tisonkun

Thanks for your updates @kl0u! Generally looks good to me.

This pull request has conflicts with current master and fails on compile error. Please rebase and fix compile error.

…plan executions.

kl0u · 2019-09-04T20:19:29Z

I integrated your comments @aljoscha . Please have a look and let me know what you think.

tisonkun · 2019-09-05T06:19:27Z

flink-yarn-tests/src/test/java/org/apache/flink/yarn/util/FakeClusterClient.java

@@ -93,4 +93,9 @@ public String stopWithSavepoint(JobID jobId, boolean advanceToEndOfEventTime, @N
 	public Map<String, OptionalFailure<Object>> getAccumulators(JobID jobID, ClassLoader loader) {
 		return Collections.emptyMap();
 	}
+
+	@Override
+	public void close() throws Exception {


Wried. The latest CI complaint with

/home/travis/build/flink-ci/flink/flink-yarn-tests/src/test/java/org/apache/flink/yarn/util/FakeClusterClient.java:[43,8] org.apache.flink.yarn.util.FakeClusterClient is not abstract and does not override abstract method close() in java.lang.AutoCloseable

but actually we implement it.

Let's see what travis says now.

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

tisonkun · 2019-09-05T06:23:44Z

Hi @kl0u, thanks for your update. I notice the strange state in travis that reports a compile error which should not be there. Please take a look.

Also you can mark conversations that addressed or reached a consensus above as "resolved" to fold them, which make ongoing reviews more clear.

tisonkun

Hi @kl0u, this pull request now looks good to me.

+1 to merge. Also it will unblock FLINK-13961 which I'm glad to implement our discussion on JobExecutor(Service). Please also take a look at the JIRA. Thanks.

aljoscha

I think this looks good now!

kl0u · 2019-09-06T13:29:25Z

Thanks for the reviews @tisonkun and @aljoscha . Merged.

kl0u requested review from tillrohrmann and aljoscha September 4, 2019 07:52

rmetzger added the review=description? label Sep 4, 2019

rmetzger added the component=Client/JobSubmission label Sep 4, 2019

tisonkun requested changes Sep 4, 2019

View reviewed changes

aljoscha requested changes Sep 4, 2019

View reviewed changes

tisonkun requested changes Sep 4, 2019

View reviewed changes

kl0u force-pushed the remove-session branch from 833e15c to 5d0397b Compare September 4, 2019 18:55

kl0u added 6 commits September 4, 2019 21:48

[FLINK-13946] Remove session-related code from execution env

3f56caf

[FLINK-13946] Remove session-related code from the PlanExecutors

39d20b0

[FLINK-13946] Refactor executor code to ban resource sharing between …

82bbc8b

…plan executions.

[FLINK-13946] Remove unused constructors from RemoteExecutor

e34eb15

LocalExecutor: rename to plan

8efb92c

Inline start and stopn in the RemoteExecutor

e94b1b5

kl0u force-pushed the remove-session branch from 5d0397b to d256fa7 Compare September 4, 2019 20:01

kl0u added 4 commits September 5, 2019 06:41

Make the ClusterClient autocloseable

c504426

refactor local executor

5147d58

Updated ContextEnv.toString

c3b1ba5

LocalExecutor remove taskManagerNumSlots

3814699

kl0u force-pushed the remove-session branch from d256fa7 to 3814699 Compare September 5, 2019 04:41

tisonkun reviewed Sep 5, 2019

View reviewed changes

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

kl0u added 2 commits September 5, 2019 09:13

Remove defaultOverwriteFiles from LocalExecutor.

8849197

Added close() to the ClusterClient.

5267206

tisonkun approved these changes Sep 6, 2019

View reviewed changes

aljoscha approved these changes Sep 6, 2019

View reviewed changes

kl0u closed this Sep 6, 2019

kl0u deleted the remove-session branch January 28, 2020 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-13946] Remove job session related code from ExecutionEnvironment #9607

[FLINK-13946] Remove job session related code from ExecutionEnvironment #9607

kl0u commented Sep 4, 2019 •

edited

flinkbot commented Sep 4, 2019 •

edited

flinkbot commented Sep 4, 2019 •

edited

tisonkun left a comment

tisonkun Sep 4, 2019

kl0u commented Sep 4, 2019

aljoscha left a comment

aljoscha Sep 4, 2019

kl0u Sep 4, 2019

tisonkun left a comment

kl0u commented Sep 4, 2019

tisonkun Sep 5, 2019

kl0u Sep 5, 2019

tisonkun commented Sep 5, 2019

tisonkun left a comment

aljoscha left a comment

kl0u commented Sep 6, 2019

[FLINK-13946] Remove job session related code from ExecutionEnvironment #9607

[FLINK-13946] Remove job session related code from ExecutionEnvironment #9607

Conversation

kl0u commented Sep 4, 2019 • edited

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

flinkbot commented Sep 4, 2019 • edited

Automated Checks

Review Progress

flinkbot commented Sep 4, 2019 • edited

CI report:

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun Sep 4, 2019

Choose a reason for hiding this comment

kl0u commented Sep 4, 2019

aljoscha left a comment

Choose a reason for hiding this comment

aljoscha Sep 4, 2019

Choose a reason for hiding this comment

kl0u Sep 4, 2019

Choose a reason for hiding this comment

tisonkun left a comment

Choose a reason for hiding this comment

kl0u commented Sep 4, 2019

tisonkun Sep 5, 2019

Choose a reason for hiding this comment

kl0u Sep 5, 2019

Choose a reason for hiding this comment

tisonkun commented Sep 5, 2019

tisonkun left a comment

Choose a reason for hiding this comment

aljoscha left a comment

Choose a reason for hiding this comment

kl0u commented Sep 6, 2019

kl0u commented Sep 4, 2019 •

edited

flinkbot commented Sep 4, 2019 •

edited

flinkbot commented Sep 4, 2019 •

edited