Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-14854][client] Add executeAsync() method to execution environments #10392

Closed
wants to merge 2 commits into from

Conversation

@TisonKun
Copy link
Contributor

TisonKun commented Dec 3, 2019

What is the purpose of the change

Add a new executeAsync() method which returns a future of JobClient. This exposes the new executor/job client work on the user API.

I added such methods in ExecutionEnvironment & StreamExecutionEnvironment and export as Scala API.

However, I don't patch code for make it works in every environment that hijacks execution method. The reason is that 1) future work will port (Local|Remote)Environment under new Executor abstraction so then they will work. 2) CollectionEnvironment doesn't support something like JobClient. Actually I think such environment can be replaced with LocalEnvironment.

Currently, I do only check not null for ExecutorFactory in executeAsync and throw an exception with description when we cannot find one, i.e., Local, Remote, Collection Environment. It will hints users that these envs don't support such feature(now).

Verifying this change

executeAsync() is a lightweight interface for default parameter. And with this change all codepath previously goes into execute(jobName) now also goes into executeAsync(jobName). Thus I think the change is already 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): (yes, we introduces more user APIs)
  • 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)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (JavaDocs, we can add document on site in the future)

cc @aljoscha @kl0u

@@ -42,11 +42,6 @@ public JobExecutionResult getLastJobExecutionResult() {
}
}

@Override
public JobExecutionResult execute() throws Exception {

This comment has been minimized.

Copy link
@TisonKun

TisonKun Dec 3, 2019

Author Contributor

IMO execute() should be a thin interface for adding default parameter, so we should mark it as final and this override makes non-sense.

This comment has been minimized.

Copy link
@TisonKun

TisonKun Dec 3, 2019

Author Contributor

hmmm mark method as final breaks b/w comp. but I think it can be removed on the fly for these meaningless override.

@flinkbot

This comment has been minimized.

Copy link

flinkbot commented Dec 3, 2019

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 8db22c8 (Wed Dec 04 15:56:40 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
@TisonKun TisonKun force-pushed the TisonKun:FLINK-14854 branch 2 times, most recently from 7b12c13 to 51e6e81 Dec 3, 2019
@kl0u kl0u self-assigned this Dec 3, 2019
@TisonKun TisonKun force-pushed the TisonKun:FLINK-14854 branch 2 times, most recently from fd0d948 to 36cb8ff Dec 3, 2019
@flinkbot

This comment has been minimized.

Copy link

flinkbot commented Dec 3, 2019

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run travis re-run the last Travis build
Copy link
Contributor

kl0u left a comment

Thanks for the work @TisonKun . I had some small comments that will take 5 min to integrate and then I think I will give a +1 after having a final look.

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

TisonKun commented Dec 3, 2019

Thanks for your review @kl0u . I've integrated your comments. Feel free to give a final check :-)

@TisonKun TisonKun force-pushed the TisonKun:FLINK-14854 branch from a830a25 to 8db22c8 Dec 3, 2019
@kl0u

This comment has been minimized.

Copy link
Contributor

kl0u commented Dec 3, 2019

Thanks a lot for the work @TisonKun ! I already merged this :)

@kl0u
kl0u approved these changes Dec 3, 2019
@kl0u kl0u closed this Dec 3, 2019
@TisonKun TisonKun deleted the TisonKun:FLINK-14854 branch Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.