[FLINK-12370][python][travis] Integrated Travis for Python Table API. #8392

WeiZhong94 · 2019-05-10T01:51:07Z

What is the purpose of the change

This pull request integrates travis for Python Table API.

Brief change log

Add new stage option in travis_controller.sh to enable python tests
Adjust minimizeCachedFiles logic to reserve files under flink-dist directory and test jars.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

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)

flinkbot · 2019-05-10T01:51:19Z

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.
- Approved by @sunjincheng121 [committer]
✅ 2. There is [consensus] that the contribution should go into to Flink.
- Approved by @sunjincheng121 [committer]
❓ 3. Needs [attention] from.
✅ 4. The change fits into the overall [architecture].
- Approved by @sunjincheng121 [committer]
✅ 5. Overall code [quality] is good.
- Approved by @sunjincheng121 [committer]

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

dianfu

@WeiZhong94 Thanks a lot for the PR. Have left a few comments.

dianfu · 2019-05-13T01:02:40Z

tools/travis/stage.sh

@@ -23,6 +23,7 @@ STAGE_LIBRARIES="libraries"
 STAGE_CONNECTORS="connectors"
 STAGE_TESTS="tests"
 STAGE_MISC="misc"
+STAGE_PYTHON="python"


Put this line under STAGE_CORE="core" to keep the order same in .travis.yml

dianfu · 2019-05-13T01:23:44Z

tools/travis_python_watchdog.sh

+# limitations under the License.
+################################################################################
+
+HERE="`dirname \"$0\"`" # relative


Most of the scripts of this file is same as travis_mvn_watchdog.sh. The only difference is how to execute the tests. What about moving the changes here to travis_mvn_watchdog.sh and do some refactoring of travis_mvn_watchdog.sh to make it work for both java and python? Maybe we should also rename travis_mvn_watchdog.sh to travis_watchdog.sh?

That makes sense. I have combined them in the new commit.

…der stage variable

WeiZhong94 · 2019-05-13T10:08:35Z

@dianfu Thanks for your review! I have addressed your comments.

dianfu

@WeiZhong94 Thanks a lot for the update. The new PR LGTM. Have left just one minor comments.

dianfu · 2019-05-13T10:10:54Z

tools/travis_watchdog.sh

@@ -78,6 +83,24 @@ UPLOAD_SECRET_KEY=$ARTIFACTS_AWS_SECRET_KEY

 ARTIFACTS_FILE=${TRAVIS_JOB_NUMBER}.tar.gz

+if [ $TEST == $STAGE_PYTHON ]; then
+


remove the empty line

sunjincheng121

Thanks for the PR. @WeiZhong94
LGTM. +1 to merged.
Best,Jincheng

sunjincheng121 · 2019-05-13T23:50:24Z

I find one thing we need double check, is the cost of the testing, maybe there is a bug need fix(I am not sure, I will check it):

@WeiZhong94

sunjincheng121 · 2019-05-13T23:53:52Z

We have two options for Python integration with Travis.

It is the addition of a python stage that PR currently reflects. Instead of using MVN for testing, it runs a separate shell.
Using MVN to execute the shell, Python validation is also MVN is tested. I personally prefer the current PR program. The reason is that:
- The Python API is a different language from Java/Scala. The code management and testing methods are not dependent on MVN. Python multi-version and Python API functional tests are combined using Conda and Tox. So we choose not to use the MVN solution;
- The Python API is a new language API that has just been incorporated into Flink. We would like to have an intuitive understanding of Python's details in the Travis process and intuitively observe the time spent testing the Python API. So choose a separate stage and a separate run output solution.

Of course, there are 2 aspects, 1. Independent stage 2. Is it integrated into MVN, that is, 2x2=4 options, I tend to the current PR.

What do you think? @aljoscha @zentol

sunjincheng121 · 2019-05-14T01:40:09Z

I find one thing we need double check, is the cost of the testing, maybe there is a bug need fix(I am not sure, I will check it):

@WeiZhong94

the script bug, see follows:

WeiZhong94 · 2019-05-14T03:22:19Z

@sunjincheng121 Thanks for your review! Sorry for these mistakes. I have fixed them in the new commit and ensure the travis works correctly.

…sage after python test finished.

sunjincheng121 · 2019-05-15T01:12:51Z

Thanks for the fix @WeiZhong94
LGTM. +1 to merged
@flinkbot approve all

Brief change log: - Added python stage for Python API travis testing. - Integrated flink-python/dev/lint-python.sh for CI testing of the Python API. This closes apache#8392

[FLINK-12370][python][travis] Integrated Travis for Python Table API.

861423c

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

WeiZhong94 added 3 commits May 10, 2019 11:18

add python watchdog

527eedc

fix typo

d6eafab

remove unnecessary change

d46abad

dianfu reviewed May 13, 2019

View reviewed changes

combine travis_mvn_watchdog.sh and travis_python_watchdog.sh and reor…

3704d00

…der stage variable

dianfu reviewed May 13, 2019

View reviewed changes

sunjincheng121 reviewed May 13, 2019

View reviewed changes

fix travis watchdog's bug

84f31c1

change the if statements structure so that it won't print failure mes…

52d6fca

…sage after python test finished.

asfgit closed this in d95d395 May 15, 2019

rmetzger added review=approved ✅ and removed review=description? labels May 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-12370][python][travis] Integrated Travis for Python Table API. #8392

[FLINK-12370][python][travis] Integrated Travis for Python Table API. #8392

WeiZhong94 commented May 10, 2019

flinkbot commented May 10, 2019 •

edited

Loading

dianfu left a comment

dianfu May 13, 2019

WeiZhong94 May 13, 2019

dianfu May 13, 2019

WeiZhong94 May 13, 2019

WeiZhong94 commented May 13, 2019

dianfu left a comment

dianfu May 13, 2019

sunjincheng121 left a comment

sunjincheng121 commented May 13, 2019

sunjincheng121 commented May 13, 2019

sunjincheng121 commented May 14, 2019

WeiZhong94 commented May 14, 2019

sunjincheng121 commented May 15, 2019

		@@ -78,6 +83,24 @@ UPLOAD_SECRET_KEY=$ARTIFACTS_AWS_SECRET_KEY

		ARTIFACTS_FILE=${TRAVIS_JOB_NUMBER}.tar.gz

		if [ $TEST == $STAGE_PYTHON ]; then

[FLINK-12370][python][travis] Integrated Travis for Python Table API. #8392

[FLINK-12370][python][travis] Integrated Travis for Python Table API. #8392

Conversation

WeiZhong94 commented May 10, 2019

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

Review Progress

dianfu left a comment

Choose a reason for hiding this comment

dianfu May 13, 2019

Choose a reason for hiding this comment

WeiZhong94 May 13, 2019

Choose a reason for hiding this comment

dianfu May 13, 2019

Choose a reason for hiding this comment

WeiZhong94 May 13, 2019

Choose a reason for hiding this comment

WeiZhong94 commented May 13, 2019

dianfu left a comment

Choose a reason for hiding this comment

dianfu May 13, 2019

Choose a reason for hiding this comment

sunjincheng121 left a comment

Choose a reason for hiding this comment

sunjincheng121 commented May 13, 2019

sunjincheng121 commented May 13, 2019

sunjincheng121 commented May 14, 2019

WeiZhong94 commented May 14, 2019

sunjincheng121 commented May 15, 2019

flinkbot commented May 10, 2019 •

edited

Loading