[BEAM-9764] multi threading & optional pulling #11428
Conversation
|
Run Python2_PVR_Flink PreCommit |
e4e98f1
to
3ea2eea
Compare
|
Run Python2_PVR_Flink PreCommit |
46a34a8
to
e716a47
Compare
94a5dbc
to
4401e4d
Compare
4401e4d
to
cf238c4
Compare
|
R: @udim |
yes |
| cp -r java_third_party_licenses/*.jar sdks/java/container/third_party_licenses/ | ||
| cp -r java_third_party_licenses/*.csv sdks/java/container/third_party_licenses/ | ||
| else | ||
| # create an empty file to aviod no file/dir existing error |
There was a problem hiding this comment.
| # create an empty file to aviod no file/dir existing error | |
| # create an empty file to avoid no file/dir existing error |
| @@ -25,15 +25,23 @@ pip install PyYAML==5.3 | |||
| pip install tenacity==5.0.4 | |||
There was a problem hiding this comment.
BTW, I wanted to mention that doing a pip install/uninstall like this messes with the user's environment.
The standard way to run this would be via tox (see sdks/python/tox.ini), which will take care of creating a virtualenv to run this under (and you can specify a certain python version if you wish).
There was a problem hiding this comment.
I reviewed to use tox here, but it is a little complicated. We want to keep it simple within a script, so it's easy to use and maintain. Current scripts run at local machine, (not within a docker), so docker will still have clear environment.
There was a problem hiding this comment.
I don't agree that it's acceptable to modify the user environment. Also, on Jenkins this might cause failure if this script runs concurrently with itself.
I've opened a bug for this: https://issues.apache.org/jira/browse/BEAM-9797
There was a problem hiding this comment.
Thank you Udi for bringing up this issue. Yes, we should fix it. I will merge this PR and work on a different PR to fix it.
| no_license_type.add(name_version) | ||
| license_type = '' | ||
| continue | ||
| csv_list = [] |
There was a problem hiding this comment.
These lists need to have synchronized access if they are to be modified by different threads.
https://stackoverflow.com/questions/2227169/are-python-built-in-containers-thread-safe
There was a problem hiding this comment.
According to this (post)[https://stackoverflow.com/questions/6319207/are-lists-thread-safe], append operation is thread safe. But I added lock to make sure all records are added and the operation is not expensive.
408511d
to
06cb50e
Compare
|
Thanks Udi for reviewing it. I added some replies to your comments and also fixed some of them. Could you please take a look? |
|
Run Java PreCommit |
1 similar comment
|
Run Java PreCommit |
* multi threading & check urls instead of pulling * add thread_lock and more logs * fix package version and import
* multi threading & check urls instead of pulling * add thread_lock and more logs * fix package version and import
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.