TheNeuralBit commented on a change in pull request #12505:
URL: https://github.com/apache/beam/pull/12505#discussion_r488809349
##########
File path: sdks/java/container/Dockerfile-java11
##########
@@ -16,6 +16,10 @@
# limitations under the License.
###############################################################################
+##
+# NOTE: This image is now deprecated.
+# Use Dockerfile with the appropriate java_version build argument.
+##
Review comment:
I'm +1 for just removing this. Looks like it was added in
https://github.com/apache/beam/pull/8053 in order to start running a job for
testing Java 11. As long as the jobs that use Java 11 still pass after removing
this I think it's fine.
I don't think users should be referencing this Dockerfile directly, if
they're building a Java 11 container at all it should be through the gradle
command (and even then we don't support Java 11 so they shouldn't be doing it).
##########
File path: website/www/site/content/en/documentation/runtime/environments.md
##########
@@ -116,8 +116,8 @@ By default, no licenses/notices are added to the docker
images.
To examine the containers that you built, run `docker images` from anywhere in
the command line. If you successfully built all of the container images, the
command prints a table like the following:
```
-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
Review comment:
typo here:
```suggestion
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?
##########
File path:
runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/Environments.java
##########
@@ -357,4 +354,14 @@ private static File zipDirectory(File directory) throws
IOException {
return env;
}
}
+
+ private static String getDefaultJavaSdkHarnessContainerUrl() {
+ String javaVersionId =
+ Float.parseFloat(System.getProperty("java.specification.version")) >=
9 ? "java11" : "java8";
Review comment:
nit: could you make this an exact check for 8 and 11 and throw an
exception for other (unsupported) versions
##########
File path: sdks/java/container/Dockerfile-java11
##########
@@ -16,6 +16,10 @@
# limitations under the License.
###############################################################################
+##
+# NOTE: This image is now deprecated.
+# Use Dockerfile with the appropriate java_version build argument.
+##
Review comment:
Also the fact that this Dockerfile doesn't copy LICENSE and NOTICE is
problematic.
----------------------------------------------------------------
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:
us...@infra.apache.org
