[FLINK-12541][REST] Support to submit Python Table API jobs via REST API #8532

dianfu · 2019-05-24T08:05:11Z

What is the purpose of the change

This pull request supports to submit Python Table API jobs via REST API.
To support submit resource files other than jar, we need to update the REST API a bit as specified in FLIP38

Current REST API	New REST API	Remarks
GET /jars	GET /artifacts	Now it lists both jar files and python package files.
POST /jars/upload	POST /artifacts/upload	Allow users to upload both jar files and python package files. Use "Content-Type" in the request header to mark the file type.
DELETE /jars/:jarid	DELETE /artifacts/:artifactid	Delete files according to “artifactid”.
GET /jars/:jarid/plan	GET /artifacts/:artifactid/plan	We will introduce a new optional parameter:pythonEntryModule.It allows specifying a python entry point module other than main.py.
POST /jars/:jarid/run	POST /artifacts/:artifactid/run	We also introduce the optional pythonEntryModule parameter here, as same as above.

Brief change log

2c4d575: Changes the REST API prefix from /jars to /artifacts as we will not only support upload jar files, but also Python resource files. This commit also refactor the class name prefix from JarXXX to ArtifactXXX
c286149: Updates the web to use the new /artifacts REST API instead of the /jars REST API
b516468 Supports to submit Python Table API jobs. It supports to submit Python Table API jobs via resource files such as .py, .zip and .egg.

Verifying this change

This change added tests and can be verified as follows:

Added tests ArtifactHandlerUtilsTest.testExtractContainedLibraries, ArtifactXXXTest

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

flinkbot · 2019-05-24T08:06:46Z

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.

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❗ 3. Needs [attention] from.
- Needs attention by @tillrohrmann [PMC], @zentol [PMC]
❓ 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

sunjincheng121 · 2019-05-28T07:41:55Z

Thanks for your PR. @dianfu!
I have merged the https://issues.apache.org/jira/browse/FLINK-12327, so please rebase the code :) then I'll review this PR ASAP.
Best,
Jincheng

dianfu · 2019-05-28T08:03:13Z

@sunjincheng121 Great, I have rebased the PR.

sunjincheng121 · 2019-05-29T01:44:42Z

@flinkbot attention @tillrohrmann @zentol

tillrohrmann · 2019-06-04T09:43:40Z

Since you are changing part of Flink's public API, how did you check that we did not break backwards compatibility @dianfu, @sunjincheng121? It should still be possible to submit jobs with a 1.8 client against a 1.9 cluster.

sunjincheng121 · 2019-06-04T09:51:52Z

That's a good point, @tillrohrmann! We use the org.apache.flink.runtime.rest.compatibility.RestAPIStabilityTest to ensure compatibility. and in current PR should keep the old APIs there but mark Deprecated for the old APIs. Do you think this is enough? Or we still need to do other extra work ?

zentol

I'd like to rework this PR a bit.
To ensure better backwards compatibility I would suggest to not touch existing APIs/handlers at all, and have the new APIs/handlers be completely separate from the old ones. This implies reverting the renaming of files and any modifications done, specifically to the JarRequestBody ~~which actually breaks backwards-compatibilty~~. All classes from the old API can be deprecated.

dianfu · 2019-06-04T10:03:22Z

@tillrohrmann Thanks a lot for your reply. As mentioned by @sunjincheng121 that the test case RestAPIStabilityTest can make sure the backwards compatibility. Besides, I have also manually checked that the old REST API still works by the following way:

Only apply the first commit which adds REST API /artifacts/ and deprecates the old REST API /jars/
Build the project, start up a local cluster and check that the functionalities such as uploading jars, getting plans, submitting jobs works well.
As only the first commit is applied, the frontend still uses the old REST API. This makes sure that the old REST API still works.

dianfu · 2019-06-04T10:06:49Z

@zentol I think your suggestions make sense to me. Will updated the PR accordingly. Thanks a lot.

zentol · 2019-06-04T10:11:51Z

@dianfu In your manual test, did you not include commit #3? That one modifies the API as well, and from what I can tell at a glance should also impact the legacy /jar endpoints.
Now that we separate things though this should no longer matter.

dianfu · 2019-06-04T10:19:57Z

@zentol The manual test does not include the third commit. Definitely agree that separating the new API with the old API will be more safer for backwards compatibility. :)

dianfu · 2019-06-04T12:09:51Z

@zentol @tillrohrmann I have updated the PR and separated the newly added API with the old ones per @zentol 's suggestion. Could you help to take a look at? Appreciated!

tillrohrmann · 2019-06-05T12:33:51Z

RestAPIStabilityTest.testDispatcherRestAPIStability is failing the build @dianfu.

dianfu · 2019-06-06T03:30:07Z

@tillrohrmann Thanks a lot for the remind and have addressed the test failure.

sunjincheng121

Thanks for the updated @dianfu!
I have left a few suggestions, and two question as follows:

At present, there are some ArtifactXXX and JarXXX content is almost 90% identical. The reason we directly copy one is to facilitate the next release deletion? such as ArtifactListInfo and JarListInfo and ArtifactRequestBody and JarRequestBody, (only add new property dependentArtifactId) etc.
For the test case, we just rename the JarxxxTest to ArtifactxxxTest and test the Artifactxxx logic. But how do we deal with the logic test for Jarxxx? if we want to do some bugfix for Jarxxx?
Please let me know what do you think?
Best, Jincheng

sunjincheng121 · 2019-06-10T01:23:40Z

.../src/main/java/org/apache/flink/runtime/webmonitor/handlers/AbstractArtifactPlanHeaders.java

+
+ @Override
+ public String getTargetRestEndpointURL() {
+ return "/artifacts/:" + ArtifactIdPathParameter.KEY + "/plan";


Can we add a constant for artifacts due to there are many places using this string? such as ArtifactDeleteHeaders, ArtifactListHeadersetc.
I know the old one AbstractJarPlanHeaders also using the jars string directly, but I
think we can do some improvement if it makes sense to us. What do you think?

That makes sense to me. +1

sunjincheng121 · 2019-06-10T01:58:13Z

...time-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/ArtifactRequestBody.java

+
+ @Nullable
+ @JsonIgnore
+ public String getDependentArtifactId() {


Why we should add this new method?

This parameter allows to specify a separate jar file as the dependency. What do you think?

Yes, I know we should have a way to let the user upload their fat JAR for Python Job. Furthermore, we also need a way to let the user upload the resources of pythons files, so we need a ZIP folder structure, such as :
zip:
/lib ---> for JAR and python files ZIPs.
/xx/wordcount.py ---> user code
What do you think?

sunjincheng121 · 2019-06-10T02:37:33Z

...test/java/org/apache/flink/runtime/webmonitor/handlers/ArtifactPlanHandlerParameterTest.java

 return handler.getMessageHeaders().getUnresolvedMessageParameters();
 }

 @Override
- JarPlanMessageParameters getJarMessageParameters(ProgramArgsParType programArgsParType) {
- final JarPlanMessageParameters parameters = getUnresolvedJarMessageParameters();
+ ArtifactPlanMessageParameters getJarMessageParameters(ProgramArgsParType programArgsParType) {


getJarMessageParameters -> getArtifactMessageParameters?

sunjincheng121 · 2019-06-10T02:39:08Z

...test/java/org/apache/flink/runtime/webmonitor/handlers/ArtifactPlanHandlerParameterTest.java

 */
-public class JarPlanHandlerParameterTest extends JarHandlerParameterTest<JarPlanRequestBody, JarPlanMessageParameters> {
- private static JarPlanHandler handler;
+public class ArtifactPlanHandlerParameterTest


replace all jar with Artifact? such as getWrongJarMessageParameters -> getWrongArtifactMessageParameters?

sunjincheng121 · 2019-06-10T02:39:33Z

...test/java/org/apache/flink/runtime/webmonitor/handlers/ArtifactPlanHandlerParameterTest.java

@@ -94,27 +95,28 @@ JarPlanMessageParameters getWrongJarMessageParameters(ProgramArgsParType program
 }

 @Override
- JarPlanRequestBody getDefaultJarRequestBody() {
- return new JarPlanRequestBody();
+ ArtifactPlanRequestBody getDefaultJarRequestBody() {


getDefaultJarRequestBody -> getDefaultArtifactRequestBody

sunjincheng121 · 2019-06-10T03:05:21Z

flink-runtime-web/web-dashboard/src/app/pages/submit/submit.component.ts

@@ -154,7 +154,7 @@ export class SubmitComponent implements OnInit, OnDestroy {
 }

 constructor(
- private jarService: JarService,
+ private artifactService: ArtifactService,


Line 152: trackJarBy -> trackArtifactBy?

sunjincheng121 · 2019-06-10T03:05:42Z

flink-runtime-web/web-dashboard/src/app/pages/submit/submit.component.html

- <tr (click)="expandJar(jar)" class="clickable">
- <td>{{jar.name}}</td>
- <td>{{jar.uploaded | date:'yyyy-MM-dd, HH:mm:ss'}}</td>
+ <ng-container *ngFor="let artifact of listOfJar; trackBy:trackJarBy;">


trackJarBy -> trackArtifactBy

sunjincheng121 · 2019-06-10T03:06:43Z

flink-runtime-web/web-dashboard/src/app/pages/submit/submit.component.ts

 */
- deleteJar(jar: JarFilesItemInterface) {
- this.jarService.deleteJar(jar.id).subscribe(() => {
+ deleteArtifact(artifact: JarFilesItemInterface) {


JarFilesItemInterface -> ArtifactFilesItemInterface?

sunjincheng121 · 2019-06-10T03:10:06Z

flink-runtime-web/web-dashboard/src/app/pages/submit/submit.component.ts

@@ -23,7 +23,7 @@ import { Router } from '@angular/router';
 import { JarFilesItemInterface } from 'interfaces';


should we rename the JarFilesItemInterface to ArtifactFilesItemInterface in jar.js?

sunjincheng121 · 2019-06-10T03:11:24Z

flink-runtime-web/web-dashboard/src/app/services/artifact.service.ts

- loadJarList() {
- return this.httpClient.get<JarListInterface>(`${BASE_URL}/jars`).pipe(
+ loadArtifactList() {
+ return this.httpClient.get<JarListInterface>(`${BASE_URL}/artifacts`).pipe(


Should we rename the JarListInterface to ArtifactListInterface in jar.js?

I think so. +1

dianfu · 2019-06-10T14:50:19Z

@sunjincheng121 Thanks a lot for the review.

Regarding to question 1, it's to ensure better backwards compatibility per @zentol 's suggestion.
Regarding to question 2, adding separate test cases for ArtifactXXX seems reasonable for me. We can remove the JarXXX and the corresponding test cases in the next releases together.

Will update the PR later on.

dianfu · 2019-06-11T03:10:46Z

@sunjincheng121 Have updated the PR according to your comments. Looking forwarding to your feedback.

sunjincheng121

Thanks for the quick update @dianfu!
I think one important thing we should fix in this JIRA is how to upload the user JAR and python files. I left one comment about this functionality. i.e. we need a ZIP folder structure, such as :
zip:
/lib ---> for JAR and python files ZIPs.
/xx/wordcount.py ---> user code
What do you think?
Best,
Jincheng

sunjincheng121 · 2019-06-11T06:34:59Z

...time-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/ArtifactRequestBody.java

+
+ @Nullable
+ @JsonIgnore
+ public String getDependentArtifactId() {


Yes, I know we should have a way to let the user upload their fat JAR for Python Job. Furthermore, we also need a way to let the user upload the resources of pythons files, so we need a ZIP folder structure, such as :
zip:
/lib ---> for JAR and python files ZIPs.
/xx/wordcount.py ---> user code
What do you think?

dianfu · 2019-06-11T13:52:08Z

@sunjincheng121 I think packaging the dependencies into the zip file makes much sense to me. I have updated the PR accordingly.

sunjincheng121 · 2019-06-12T06:18:27Z

I found the time out error of CI, so I have restarted the CI.

tillrohrmann · 2019-06-12T11:42:18Z

Before diving into the review of this rather large PR, I wanted to ask whether the web based submission is a crucial feature for Flink 1.9 or not? I'm wondering whether the client based submission of Python programs wouldn't be enough as a first step.

dianfu · 2019-06-12T15:12:53Z

@tillrohrmann I agree that client based submission can meet most requirements and I'm not sure if there are users which only use REST API to submit jobs. Anyway, It's cool if we can support the submission of Python jobs for all kinds of users in 1.9.
Regarding to the large PR, actually most changes are copying the JarXXX to ArtifactXXX which are all in the first commit. The changes of the last two commits are pretty small.

sunjincheng121 · 2019-06-13T01:55:01Z

@tillrohrmann this feature not the crucial feature for Flink 1.9, it's not must-have for Flink 1.9. but I also suggest adding this change to Flink 1.9, due to the following reasons:

API refactoring: this PR is pretty big like you said, but most of the change is refactoring, i.e.
the change is: jars -> artifacts. And I think if we ensure that the change of jars -> artifacts is the right thing, I suggest do this change in Flink 1.9. :)
REST API for python: I think you are right, without REST API we can using the client to submit the Python Job, but we only need to do a small change for the support this feature(only
300+ line change). I suggest adding this to Flink 1.9. :) But I agree with you that this is nice to have, not must-have.

What do you think?

sunjincheng121 · 2019-06-20T06:05:44Z

@tillrohrmann do you still think we should not merge this change into 1.9? just double check and confirm. :)

… /artifacts

elanv · 2020-11-20T07:18:45Z

How is it going? I want to submit a python job inside application, and it would be great if REST API supports it.

flinkbot · 2020-11-20T07:35:21Z

CI report:

d24c4f7 Unknown: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

dianfu · 2020-11-23T06:16:50Z

@elanv The REST API is still not supported as we want to add support of client based submission as the first step. However, it makes sense to consider the REST API support now as more and more functionalities have already been supported for PyFlink and more and more users are trying it out. As 1.12 is already feature freeze and so I think we could not make it in the 1.12. I will revisit this work and hope we could support it in 1.13.

ikstewa · 2023-09-17T16:19:00Z

Is there still plans to support pyflink job submission via REST api?

rmetzger added review=description? component=API/Python labels May 24, 2019

dianfu force-pushed the FLINK-12541 branch from 524e206 to 28380d7 Compare May 28, 2019 07:59

dianfu force-pushed the FLINK-12541 branch from 28380d7 to d3de3b8 Compare June 3, 2019 06:39

rmetzger requested review from tillrohrmann and zentol June 3, 2019 14:10

dianfu force-pushed the FLINK-12541 branch 2 times, most recently from 8d05d07 to 569010b Compare June 4, 2019 01:22

zentol requested changes Jun 4, 2019

View reviewed changes

dianfu force-pushed the FLINK-12541 branch from 569010b to 19193c0 Compare June 4, 2019 12:05

dianfu force-pushed the FLINK-12541 branch from 19193c0 to a8186ad Compare June 5, 2019 01:33

rmetzger requested a review from zentol June 5, 2019 08:46

tillrohrmann mentioned this pull request Jun 5, 2019

[FLINK-12541][container][python] Add support for Python jobs in build script #8609

Closed

dianfu force-pushed the FLINK-12541 branch from a8186ad to d689e0c Compare June 6, 2019 01:13

sunjincheng121 reviewed Jun 10, 2019

View reviewed changes

dianfu force-pushed the FLINK-12541 branch from d689e0c to 3c373d5 Compare June 11, 2019 03:06

sunjincheng121 reviewed Jun 11, 2019

View reviewed changes

dianfu force-pushed the FLINK-12541 branch from 3c373d5 to b516468 Compare June 11, 2019 13:50

rmetzger added the component=Runtime/REST label Jun 20, 2019

dianfu added 3 commits June 20, 2019 22:13

[FLINK-12541][REST] Change the rest api from prefix /jars to /artifacts

f56e1d2

[FLINK-12541][web] Update the web to use the new rest api with prefix…

5dcc193

… /artifacts

[FLINK-12541][REST] Support to submit Python jobs via REST API

d24c4f7

dianfu force-pushed the FLINK-12541 branch from b516468 to d24c4f7 Compare June 20, 2019 14:14

elanv mentioned this pull request Nov 20, 2020

Improve submitting/tracking job and fix job recovery bug GoogleCloudPlatform/flink-on-k8s-operator#372

Closed

tillrohrmann removed their request for review June 29, 2022 09:38

		@@ -23,7 +23,7 @@ import { Router } from '@angular/router';
		import { JarFilesItemInterface } from 'interfaces';

[FLINK-12541][REST] Support to submit Python Table API jobs via REST API #8532

Are you sure you want to change the base?

[FLINK-12541][REST] Support to submit Python Table API jobs via REST API #8532

Conversation

dianfu commented May 24, 2019 • edited Loading

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 May 24, 2019 • edited Loading

Review Progress

sunjincheng121 commented May 28, 2019

dianfu commented May 28, 2019

sunjincheng121 commented May 29, 2019

tillrohrmann commented Jun 4, 2019 • edited Loading

sunjincheng121 commented Jun 4, 2019 • edited Loading

zentol left a comment • edited Loading

Choose a reason for hiding this comment

dianfu commented Jun 4, 2019

dianfu commented Jun 4, 2019

zentol commented Jun 4, 2019

dianfu commented Jun 4, 2019

dianfu commented Jun 4, 2019

tillrohrmann commented Jun 5, 2019

dianfu commented Jun 6, 2019

sunjincheng121 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

dianfu commented Jun 10, 2019

dianfu commented Jun 11, 2019

sunjincheng121 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianfu commented Jun 11, 2019

sunjincheng121 commented Jun 12, 2019

tillrohrmann commented Jun 12, 2019

dianfu commented Jun 12, 2019 • edited Loading

sunjincheng121 commented Jun 13, 2019

sunjincheng121 commented Jun 20, 2019

elanv commented Nov 20, 2020

flinkbot commented Nov 20, 2020 • edited Loading

CI report:

dianfu commented Nov 23, 2020

ikstewa commented Sep 17, 2023

dianfu commented May 24, 2019 •

edited

Loading

flinkbot commented May 24, 2019 •

edited

Loading

tillrohrmann commented Jun 4, 2019 •

edited

Loading

sunjincheng121 commented Jun 4, 2019 •

edited

Loading

zentol left a comment •

edited

Loading

dianfu commented Jun 12, 2019 •

edited

Loading

flinkbot commented Nov 20, 2020 •

edited

Loading