You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/10/28 00:48:31 UTC
[GitHub] [beam] emilymye opened a new pull request #13211: [WIP][BEAM-8106] Release Java 11 container image
emilymye opened a new pull request #13211:
URL: https://github.com/apache/beam/pull/13211
**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:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit merged pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
TheNeuralBit merged pull request #13211:
URL: https://github.com/apache/beam/pull/13211
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] emilymye commented on a change in pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
emilymye commented on a change in pull request #13211:
URL: https://github.com/apache/beam/pull/13211#discussion_r516283074
##########
File path: .test-infra/jenkins/job_PreCommit_Java_Examples_Dataflow_Java11.groovy
##########
@@ -46,7 +46,6 @@ builder.build {
gradle {
rootBuildScriptDir(properties.checkoutDir)
tasks 'javaExamplesDataflowPreCommit'
- switches '-PimageJavaVersion=11'
Review comment:
since this uses the runner buildAndPushDockerContainer task which now checks for -PcompileAndRunTestsWithJava11, we don't need this flag anymore
##########
File path: sdks/java/container/build.gradle
##########
@@ -54,17 +55,6 @@ dependencies {
dockerDependency library.java.kafka_clients
}
-task copyDockerfileDependencies(type: Copy) {
Review comment:
Docker tasks were split out into common.gradle
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #13211:
URL: https://github.com/apache/beam/pull/13211#discussion_r524528475
##########
File path: sdks/java/container/build.gradle
##########
@@ -86,51 +76,21 @@ licenseReport {
renderers = [new JsonReportRenderer()]
}
-def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion') : '8'
-docker {
Review comment:
Hm ok, I'm not sure if we have any precedent to follow for this. I don't think it's worth going to a lot of trouble for. Instead you could just send an FYI email to dev@ once this is merged letting people know they may need to change :sdks:java:container:docker -> :sdks:java:container:java8:docker.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] emilymye commented on pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
emilymye commented on pull request #13211:
URL: https://github.com/apache/beam/pull/13211#issuecomment-724211484
Run Java Dataflow V2 ValidatesRunner
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #13211:
URL: https://github.com/apache/beam/pull/13211#discussion_r520854059
##########
File path: sdks/java/container/build.gradle
##########
@@ -86,51 +76,21 @@ licenseReport {
renderers = [new JsonReportRenderer()]
}
-def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion') : '8'
-docker {
- name containerImageName(
- name: "${project.docker_image_default_repo_prefix}java${imageJavaVersion}_sdk",
- root: project.rootProject.hasProperty(["docker-repository-root"]) ?
- project.rootProject["docker-repository-root"] :
- project.docker_image_default_repo_root,
- tag: project.rootProject.hasProperty(["docker-tag"]) ?
- project.rootProject["docker-tag"] : project.sdk_version)
- dockerfile project.file("./Dockerfile")
- files "./build/"
- buildArgs([
- 'pull_licenses': project.rootProject.hasProperty(["docker-pull-licenses"]) ||
- project.rootProject.hasProperty(["isRelease"]),
- 'java_version': imageJavaVersion,
- ])
+task buildAll {
+ dependsOn ':sdks:java:container:java8:docker'
+ dependsOn ':sdks:java:container:java11:docker'
}
task pullLicenses(type: Exec) {
+ dependsOn generateLicenseReport
+ generateLicenseReport.outputs.cacheIf { false }
Review comment:
(not a blocker for this PR) @kennknowles can you comment on why we need this? It would be good to have a comment explaining it. Looks like it was added in https://github.com/apache/beam/commit/ebd65e5314eb42131eace3be62390b3570e71292
##########
File path: runners/portability/java/build.gradle
##########
@@ -214,7 +214,7 @@ def createUlrValidatesRunnerTask = { name, environmentType ->
}
if (environmentType == "DOCKER") {
- vrTask.dependsOn ":sdks:java:container:docker"
+ vrTask.dependsOn ":sdks:java:container:java8:docker"
Review comment:
Are there plans to make this and the other portable tests runnable with the Java 11 container? Seems like we should drop a TODO and/or file a jira
##########
File path: sdks/java/container/build.gradle
##########
@@ -86,51 +76,21 @@ licenseReport {
renderers = [new JsonReportRenderer()]
}
-def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion') : '8'
-docker {
Review comment:
Maybe we should keep the `:sdks:java:container:docker` task for a bit and just have it error with a message directing people to java8:docker and/or java11:docker?
##########
File path: sdks/java/container/build.gradle
##########
@@ -54,17 +55,6 @@ dependencies {
dockerDependency library.java.kafka_clients
}
-task copyDockerfileDependencies(type: Copy) {
Review comment:
Ack, thank you
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13211:
URL: https://github.com/apache/beam/pull/13211#issuecomment-728488362
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on a change in pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #13211:
URL: https://github.com/apache/beam/pull/13211#discussion_r520871132
##########
File path: sdks/java/container/common.gradle
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+applyDockerNature()
Review comment:
It would be handy to have some reverse pointers from this file to where it is used, in case things move around. Since this file is essentially meant to be used as a parameterized entrypoint for the other two modules. I know it may seem obvious but someone without context coming here will take a minute to sort it out.
##########
File path: sdks/java/container/build.gradle
##########
@@ -86,51 +76,21 @@ licenseReport {
renderers = [new JsonReportRenderer()]
}
-def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion') : '8'
-docker {
- name containerImageName(
- name: "${project.docker_image_default_repo_prefix}java${imageJavaVersion}_sdk",
- root: project.rootProject.hasProperty(["docker-repository-root"]) ?
- project.rootProject["docker-repository-root"] :
- project.docker_image_default_repo_root,
- tag: project.rootProject.hasProperty(["docker-tag"]) ?
- project.rootProject["docker-tag"] : project.sdk_version)
- dockerfile project.file("./Dockerfile")
- files "./build/"
- buildArgs([
- 'pull_licenses': project.rootProject.hasProperty(["docker-pull-licenses"]) ||
- project.rootProject.hasProperty(["isRelease"]),
- 'java_version': imageJavaVersion,
- ])
+task buildAll {
Review comment:
Unused?
##########
File path: sdks/java/container/common.gradle
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+applyDockerNature()
+
+def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion').replace("java", "") : '8'
+
+description = "Apache Beam :: SDKs :: Python :: Container :: Java ${imageJavaVersion} Container"
Review comment:
I would also suggest potentially passing in the whole atomic name. Also maybe no need for "Python" in the name.
##########
File path: sdks/java/container/java11/build.gradle
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+plugins {
+ id 'base'
+ id 'org.apache.beam.module'
+}
+
+project.ext {
+ imageJavaVersion = '11'
Review comment:
My dream, which may be impossible, is that this can be passed as a parameter somehow to the `apply from:` line so that the control and data flow are more clear.
##########
File path: sdks/java/container/build.gradle
##########
@@ -86,51 +76,21 @@ licenseReport {
renderers = [new JsonReportRenderer()]
}
-def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion') : '8'
-docker {
- name containerImageName(
- name: "${project.docker_image_default_repo_prefix}java${imageJavaVersion}_sdk",
- root: project.rootProject.hasProperty(["docker-repository-root"]) ?
- project.rootProject["docker-repository-root"] :
- project.docker_image_default_repo_root,
- tag: project.rootProject.hasProperty(["docker-tag"]) ?
- project.rootProject["docker-tag"] : project.sdk_version)
- dockerfile project.file("./Dockerfile")
- files "./build/"
- buildArgs([
- 'pull_licenses': project.rootProject.hasProperty(["docker-pull-licenses"]) ||
- project.rootProject.hasProperty(["isRelease"]),
- 'java_version': imageJavaVersion,
- ])
+task buildAll {
+ dependsOn ':sdks:java:container:java8:docker'
+ dependsOn ':sdks:java:container:java11:docker'
}
task pullLicenses(type: Exec) {
+ dependsOn generateLicenseReport
+ generateLicenseReport.outputs.cacheIf { false }
Review comment:
I noticed when I was fixing this file up that the dependency report was being pulled from the build cache. I guess it doesn't depend on the `build.gradle` so when deps change or when the config changes it doesn't update the report. I could be wrong. It may have been a temporary problem related to the particular change I was making.
##########
File path: sdks/java/container/common.gradle
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+applyDockerNature()
+
+def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion').replace("java", "") : '8'
+
+description = "Apache Beam :: SDKs :: Python :: Container :: Java ${imageJavaVersion} Container"
+
+configurations {
+ dockerDependency
+ sdkHarnessLauncher
+ pulledLicenses
+}
+
+dependencies {
+ pulledLicenses project(path: ":sdks:java:container", configuration: "pulledLicenses")
+ dockerDependency project(path: ":sdks:java:container", configuration: "dockerDependency")
+ sdkHarnessLauncher project(path: ":sdks:java:container", configuration: "sdkHarnessLauncher")
+}
+
+task copyDockerfileDependencies(type: Copy) {
+ from configurations.dockerDependency
+ rename "slf4j-api.*", "slf4j-api.jar"
+ rename "slf4j-jdk14.*", "slf4j-jdk14.jar"
+ rename 'beam-sdks-java-harness-.*.jar', 'beam-sdks-java-harness.jar'
+ rename 'beam-sdks-java-io-kafka.*.jar', 'beam-sdks-java-io-kafka.jar'
+ rename 'kafka-clients.*.jar', 'kafka-clients.jar'
+ into "build/target"
+}
+
+task copySdkHarnessLauncher(type: Copy) {
+ from configurations.sdkHarnessLauncher
+ into "build/target"
+}
+
+task copyPulledLicenses(type: Copy) {
+ from configurations.pulledLicenses
+ into "build/target"
+}
+
+task copyGolangLicenses(type: Copy) {
+ from "${project(':release:go-licenses:java').buildDir}/output"
+ into "build/target/go-licenses"
+ dependsOn ':release:go-licenses:java:createLicenses'
+}
+
+task skipPullLicenses(type: Exec) {
+ executable "sh"
+ args "-c", "mkdir -p build/target/third_party_licenses && touch build/target/third_party_licenses/skip"
+}
+
+docker {
+ name containerImageName(
+ name: "${project.docker_image_default_repo_prefix}java${imageJavaVersion}_sdk",
Review comment:
Same comment about trying to keep names a bit less constructed by string append. I can see that the Java version is used in a bunch of places that need to match, so I can accept that one being injected.
How much of this could go in e.g. `applyDockerNature(...)` ?
I don't like how all of our different types of builds are defined on one monolithic file (BeamModulePlugin) with a bunch of cross-dependencies. At some point I'd love them to be factored apart into more independent plugins. But relative to the status quo, this file does seem like another kind of "nature". So this adds another kind of composition for folks to know about. In fairness, this composition is simpler and cleaner than the monolithic file. However, this is exactly how the monolithic file came to be: we had an `apply from:...` situation that grew and grew until it was impossible to deal with.
##########
File path: sdks/java/container/common.gradle
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+applyDockerNature()
+
+def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion').replace("java", "") : '8'
Review comment:
I suggest making this non-optional and failing if the file is included by a project that did not pass the parameter.
##########
File path: runners/portability/java/build.gradle
##########
@@ -214,7 +214,7 @@ def createUlrValidatesRunnerTask = { name, environmentType ->
}
if (environmentType == "DOCKER") {
- vrTask.dependsOn ":sdks:java:container:docker"
+ vrTask.dependsOn ":sdks:java:container:java8:docker"
Review comment:
Seems just as easy to make it a parameter. You could make the `$ver` a parameter as elsewhere in this PR or you could go further and pass in the name of the target to depend on.
Pro: if anyone renames the target they can search and find the fact that it is depended on.
Con: if anyone renames the target they do have to change it in all those places so if done manually it is more work. Also the control flow reading is a little more complex.
All in all, I dislike mechanical assembly of target names, whether for definition or execution. So I prefer treating the whole target name as an atomic value and passing it in.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] emilymye commented on pull request #13211: [WIP][BEAM-8106] Release Java 11 container image
Posted by GitBox <gi...@apache.org>.
emilymye commented on pull request #13211:
URL: https://github.com/apache/beam/pull/13211#issuecomment-720745947
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13211:
URL: https://github.com/apache/beam/pull/13211#issuecomment-729006239
Run Java Dataflow V2 ValidatesRunner
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] emilymye commented on a change in pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
emilymye commented on a change in pull request #13211:
URL: https://github.com/apache/beam/pull/13211#discussion_r521899254
##########
File path: sdks/java/container/java11/build.gradle
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+plugins {
+ id 'base'
+ id 'org.apache.beam.module'
+}
+
+project.ext {
+ imageJavaVersion = '11'
Review comment:
I don't think apply from supports this (I think? gradle documentation is confusing but it seems like it's not supported via https://docs.gradle.org/current/dsl/org.gradle.api.Project.html#org.gradle.api.Project:apply(java.util.Map)) but I tried to make this slightly cleaner so these files literally only holds the parameters before applying the script
##########
File path: sdks/java/container/common.gradle
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+applyDockerNature()
+
+def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion').replace("java", "") : '8'
Review comment:
done
##########
File path: sdks/java/container/common.gradle
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+applyDockerNature()
+
+def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion').replace("java", "") : '8'
+
+description = "Apache Beam :: SDKs :: Python :: Container :: Java ${imageJavaVersion} Container"
+
+configurations {
+ dockerDependency
+ sdkHarnessLauncher
+ pulledLicenses
+}
+
+dependencies {
+ pulledLicenses project(path: ":sdks:java:container", configuration: "pulledLicenses")
+ dockerDependency project(path: ":sdks:java:container", configuration: "dockerDependency")
+ sdkHarnessLauncher project(path: ":sdks:java:container", configuration: "sdkHarnessLauncher")
+}
+
+task copyDockerfileDependencies(type: Copy) {
+ from configurations.dockerDependency
+ rename "slf4j-api.*", "slf4j-api.jar"
+ rename "slf4j-jdk14.*", "slf4j-jdk14.jar"
+ rename 'beam-sdks-java-harness-.*.jar', 'beam-sdks-java-harness.jar'
+ rename 'beam-sdks-java-io-kafka.*.jar', 'beam-sdks-java-io-kafka.jar'
+ rename 'kafka-clients.*.jar', 'kafka-clients.jar'
+ into "build/target"
+}
+
+task copySdkHarnessLauncher(type: Copy) {
+ from configurations.sdkHarnessLauncher
+ into "build/target"
+}
+
+task copyPulledLicenses(type: Copy) {
+ from configurations.pulledLicenses
+ into "build/target"
+}
+
+task copyGolangLicenses(type: Copy) {
+ from "${project(':release:go-licenses:java').buildDir}/output"
+ into "build/target/go-licenses"
+ dependsOn ':release:go-licenses:java:createLicenses'
+}
+
+task skipPullLicenses(type: Exec) {
+ executable "sh"
+ args "-c", "mkdir -p build/target/third_party_licenses && touch build/target/third_party_licenses/skip"
+}
+
+docker {
+ name containerImageName(
+ name: "${project.docker_image_default_repo_prefix}java${imageJavaVersion}_sdk",
Review comment:
If I adapted it to applyDockerNature I'd probably do it in a different PR since I'll be potentially affecting all the other containers
##########
File path: sdks/java/container/common.gradle
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+applyDockerNature()
+
+def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion').replace("java", "") : '8'
+
+description = "Apache Beam :: SDKs :: Python :: Container :: Java ${imageJavaVersion} Container"
Review comment:
I kind of like the safety that we only have to supply this description path in one place
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] emilymye commented on a change in pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
emilymye commented on a change in pull request #13211:
URL: https://github.com/apache/beam/pull/13211#discussion_r521889825
##########
File path: sdks/java/container/build.gradle
##########
@@ -86,51 +76,21 @@ licenseReport {
renderers = [new JsonReportRenderer()]
}
-def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion') : '8'
-docker {
- name containerImageName(
- name: "${project.docker_image_default_repo_prefix}java${imageJavaVersion}_sdk",
- root: project.rootProject.hasProperty(["docker-repository-root"]) ?
- project.rootProject["docker-repository-root"] :
- project.docker_image_default_repo_root,
- tag: project.rootProject.hasProperty(["docker-tag"]) ?
- project.rootProject["docker-tag"] : project.sdk_version)
- dockerfile project.file("./Dockerfile")
- files "./build/"
- buildArgs([
- 'pull_licenses': project.rootProject.hasProperty(["docker-pull-licenses"]) ||
- project.rootProject.hasProperty(["isRelease"]),
- 'java_version': imageJavaVersion,
- ])
+task buildAll {
Review comment:
split a PR but didn't move this over to the followup - will do so!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #13211:
URL: https://github.com/apache/beam/pull/13211#discussion_r524529168
##########
File path: sdks/java/container/common.gradle
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * License); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Build script containign common build tasks for Java SDK Docker images.
Review comment:
typo nit:
```suggestion
* Build script containing common build tasks for Java SDK Docker images.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on a change in pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #13211:
URL: https://github.com/apache/beam/pull/13211#discussion_r562945561
##########
File path: runners/google-cloud-dataflow-java/build.gradle
##########
@@ -246,9 +247,11 @@ def createRunnerV2ValidatesRunnerTest = { Map args ->
// task directly ('dependsOn buildAndPushDockerContainer'). This ensures the correct
// task ordering such that the registry doesn't get cleaned up prior to task completion.
task buildAndPushDockerContainer() {
- dependsOn ":sdks:java:container:docker"
+ def javaVer = project.hasProperty('compileAndRunTestsWithJava11') ? "java11" : "java8"
+ dependsOn ":sdks:java:container:${javaVer}:docker"
+ finalizedBy 'cleanUpDockerImages'
Review comment:
The `finalizeBy` definition here breaks the Kafka Performance Test[1] because it makes the docker image get cleaned up as soon as it is pushed.
We define the cleanup logic here: https://github.com/apache/beam/blame/master/runners/google-cloud-dataflow-java/build.gradle#L280-L287
[1] https://ci-beam.apache.org/job/beam_PerformanceTests_Kafka_IO/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13211:
URL: https://github.com/apache/beam/pull/13211#issuecomment-728424003
Run Java_Examples_Dataflow PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] emilymye commented on a change in pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
emilymye commented on a change in pull request #13211:
URL: https://github.com/apache/beam/pull/13211#discussion_r521703844
##########
File path: sdks/java/container/build.gradle
##########
@@ -86,51 +76,21 @@ licenseReport {
renderers = [new JsonReportRenderer()]
}
-def imageJavaVersion = project.hasProperty('imageJavaVersion') ? project.findProperty('imageJavaVersion') : '8'
-docker {
Review comment:
My current approach was just to just add a `task docker { throw ... }` block to replace the Docker plugin-created tasks, but but I can't figure out how to get the license report to ignore this task (and avoid throwing the error). This might just be a gradle question but a) should I be throwing the error differently and b) should the license report be the same task for both java8/11?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13211: [BEAM-8106] Separate Java8/11 container image build tasks
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13211:
URL: https://github.com/apache/beam/pull/13211#issuecomment-728489376
Run Java Dataflow V2 ValidatesRunner
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org