-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-21732][python] Bundle Java libraries into apache-flink-libraries #15386
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
Conversation
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 58978e2 (Sat Aug 28 11:11:50 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe 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 commandsThe @flinkbot bot supports the following commands:
|
WeiZhong94
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HuangXingBo Thanks for your PR. I have left some comments for you. In addition I think the content in docs/flinkDev/building#build_pyflink also need to be changed because it does not work on current situation.
|
|
||
| try: | ||
| exec(open(version_file).read()) | ||
| except IOError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can check if the in_flink_source is true instead of try .. except.
| To build Flink with maven you can run: | ||
| mvn -DskipTests clean package | ||
| Building the source dist is done in the flink-python directory: | ||
| cd flink-python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These scripts need to change.
| PACKAGE_DATA['pyflink.licenses'] = ['*'] | ||
|
|
||
| setup( | ||
| name='apache_flink_libraries', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use - instead of _
| author='Apache Software Foundation', | ||
| author_email='dev@flink.apache.org', | ||
| python_requires='>=3.6', | ||
| description='Apache Flink Python API', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The short description need to be changed.
| flink_lib_directory = None | ||
| flink_opt_directory = None | ||
| if 'FLINK_LIB_DIR' in os.environ: | ||
| flink_lib_directory = os.path.realpath(os.environ['FLINK_LIB_DIR']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else flink_lib_directory = os.path.join(real_flink_home, "lib")?
| if 'FLINK_LIB_DIR' in os.environ: | ||
| flink_lib_directory = os.path.realpath(os.environ['FLINK_LIB_DIR']) | ||
| if 'FLINK_OPT_DIR' in os.environ: | ||
| flink_opt_directory = os.path.realpath(os.environ['FLINK_OPT_DIR']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| @@ -0,0 +1,229 @@ | |||
| ################################################################################ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add clean logic for this package in flink-python/pom.xml
| @@ -0,0 +1,5 @@ | |||
| # Apache Flink | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add some description about this library package?
| file_elements = (root_data.getElementsByTagName("files")[0]).getElementsByTagName("file") | ||
| # extracted <files><file></file></files> | ||
| for file_element in file_elements: | ||
| source = ((file_element.getElementsByTagName('source')[0]).childNodes[0]).data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this function does not support the <includes></includes> tag and <excludes></excludes> tag. I think we need to raise Exception when the file element contains them to ensure the correctness.
54b8c72 to
c90e7a0
Compare
|
@WeiZhong94 Thanks a lot for the review. I have addressed the comments at the latest commit. |
64c4dfa to
203779a
Compare
WeiZhong94
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, +1 to Merge
1316598 to
58978e2
Compare
What is the purpose of the change
This pull request will bundle Java libraries into
apache-flink-librariesBrief change log
apache-flink-librariesand bundle Java libraries into itapache-flinkwheel packages in Azure PipelineVerifying this change
This change added tests and can be verified as follows:
apache-flink-librariesandapache-flinkin a machine with old version ofapache-flinkand run some python udf testapache-flinkwheel packages in private Azure pipelineDoes this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation