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
[BEAM-8106] Add version to java container image name #12505
Conversation
f636828
to
35ac254
Compare
Run Python2_PVR_Flink PreCommit |
retest this please |
35ac254
to
2c28eff
Compare
2c28eff
to
1c46a53
Compare
Codecov Report
@@ Coverage Diff @@
## master #12505 +/- ##
==========================================
+ Coverage 82.31% 82.33% +0.02%
==========================================
Files 455 452 -3
Lines 54596 53985 -611
==========================================
- Hits 44941 44449 -492
+ Misses 9655 9536 -119
Continue to review full report at Codecov.
|
Just checking - is this blocked on anything that we can help with? |
@kennknowles sorry for the delay - I got distracted doing some other things after there was a Python precommit error that I think just ended up being a flake. Will ping when ready! |
6595512
to
46f5df4
Compare
run java postcommit |
2d221be
to
48c1242
Compare
R: @chamikaramj @TheNeuralBit |
It looks like the spotless (Java code format) check is failing. FYI you can auto-format locally with |
Note the release manager that will likely be impacted is @robinyqiu for 2.25.0, since the 2.24.0 branch has already been cut. |
Run Dataflow PortabilityApi ValidatesRunner with Java 11 |
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.
I have a couple of minor comments and suggestions. Based on https://lists.apache.org/thread.html/rdf813078ee2728b6f26cb89c2857a844139f2c8ca78a68c69512f1ff%40%3Cdev.beam.apache.org%3E I agree that your set of post-merge tasks is sufficient.
|
||
private static String getDefaultJavaSdkHarnessContainerUrl() { | ||
String javaVersionId = | ||
Float.parseFloat(System.getProperty("java.specification.version")) >= 9 ? "java11" : "java8"; |
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.
nit: could you make this an exact check for 8 and 11 and throw an exception for other (unsupported) versions
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.
I went ahead and added an Enum to Environments, and removed similar checks in DataflowRunner
REPOSITORY TAG IMAGE ID CREATED SIZE | ||
apache/beam_java_sdk latest 16ca619d489e 2 weeks ago 550MB | ||
REPOSITORY TAG IMAGE ID CREATED SIZE | ||
apache/beam_java_8sdk latest 16ca619d489e 2 weeks ago 550MB |
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.
typo here:
apache/beam_java_8sdk latest 16ca619d489e 2 weeks ago 550MB | |
apache/beam_java8_sdk latest 16ca619d489e 2 weeks ago 550MB |
I'm assuming this change and the others like it are the result of a search to find and update all the references to beam_java_sdk
, so we don't need to worry about there being other references?
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.
Let's make sure that the correct container name is set in the environment send to the remote SDKs that use Java transforms as cross-language transforms. (We don't have integration tests setup for this by we can just try existing Kafka/SQL x-lang examples to confirm)
https://github.com/apache/beam/tree/master/sdks/python/apache_beam/examples/kafkataxi
https://github.com/apache/beam/blob/master/sdks/python/apache_beam/examples/sql_taxi.py
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 should make sure the references to beam_java_sdk
are updated in those examples too
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.
Weird! I don't know why they didn't show up in my initial ctrl-f
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.
I bet it was a race condition, those examples were added in just the last couple of months
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.
I went ahead and ran the Kafka example "from local Beam repo" without specifying the --sdk_harness_container_image_overrides to see if we should pass the correct URL, which it seems to do (based on the worker container manifest):
{
"args": [ ... ],
"image": "apache/beam_java8_sdk:2.25.0.dev",
"imagePullPolicy": "IfNotPresent",
"name": "sdk-1-0",
"volumeMounts": [ {
"mountPath": "/var/opt/google",
"name": "persist"
},
Dataflow can't actually grab that container because it doesn't exist (on hub.docker.com), so the job itself doesn't work, but it seems to be grabbing the right URL.
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.
That looks correct. Thanks.
ba2a1fa
to
00fb543
Compare
Will update once I've run the x-lang example |
retest this please |
61e90df
to
e4ed684
Compare
Run Java PreCommit |
Run Python2_PVR_Flink PreCommit |
e4ed684
to
023cb28
Compare
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, thank you!
I think this is safe to merge but maybe we should hold off until INFRA-20866 is closed. If that takes a while I'd hate to have it hold up the release.
023cb28
to
d0ec608
Compare
@TheNeuralBit it looks like one of the python unit tests flake-failed? but i think this PR is ready to go |
Filed a jira for the flake: BEAM-10987 |
@TheNeuralBit composing an email now |
Thank you! |
* add java_version arg for building Java harness container * change default java container name to java8 * docs changes * delete Java11 Dockerfile * fix typo * add JavaVersion to Environments * fix more references in documentation to beam_java_sdk * fix names of functions * remove underscore
Ack. Thanks!
…On Tue, Sep 29, 2020, 5:38 PM Brian Hulette ***@***.***> wrote:
Thank you!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12505 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKDOFM2FUATUGA2Z7EPJBLSIJ4ZBANCNFSM4PYC3INA>
.
|
Adding version number to Java 8 image names (in anticipation of releasing java11 container)
This PR:
apache/beam_java8_sdk
instead ofapache/beam_java_sdk
Old:
New:
The actual difference betweeen Dockerfile-java11 and Dockerfile are minimal - literally these COPY lines:
beam/sdks/java/container/Dockerfile
Lines 35 to 36 in 63f54fd
are the only difference, and I'm pretty sure they should be copied appropriately for the Java11 image as well.
Things that should be done at submission/after merge:
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-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.md
with 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.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.