-
Notifications
You must be signed in to change notification settings - Fork 904
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
Update to OpenJDK11 in Docker image #2433
Update to OpenJDK11 in Docker image #2433
Conversation
@ravisharda @sijie please take a look. Integration tests are passing, they are not using the 'official' image but this one that uses JDK11 as well |
@@ -20,20 +20,20 @@ | |||
FROM centos:7 | |||
MAINTAINER Apache BookKeeper <[email protected]> | |||
|
|||
ARG BK_VERSION=4.9.0 | |||
ARG BK_VERSION=4.11.0 |
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.
This also upgrades the image to use 4.11.0, apart from changing the Java version. Would it make sense to separate the two and merge it to older release lines, so that users using older versions of Bookkeeper like v4.9.2 can also benefit from the Java upgrade?
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 am not sure that the bash files on 4.9.2 work with java11 due to removed JVM command line flags.
4.10 should work.
Btw we are close to releasing 4.12 so 4.9 is becoming far distant from current 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.
Also It would be better to use the version in pom files and in current master it should be 4.12.0-SNAPSHOT but if I change it the image cannot be built so currently I can only use this version.
We are in the middle of the process of releasing 4 11.1 so probably we will set 4.11.1 here on current master.
Another solution would be to pick the binaries from local filesystem if the version is a SNAPSHOT
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.
This is my fix for using JRE
that said, it won't be available to old BK versions
#2436
with BK 4.12.0 we will be able to use JRE 11
@@ -13,7 +13,7 @@ Bookkeeper needs [Zookeeper](https://zookeeper.apache.org/) in order to preserve | |||
Just like running a BookKeeper cluster in one machine(http://bookkeeper.apache.org/docs/latest/getting-started/run-locally/), you can run a standalone BookKeeper in one docker container, the command is: | |||
``` | |||
docker run -it \ | |||
--env JAVA_HOME=/usr/lib/jvm/jre-1.8.0 \ | |||
--env JAVA_HOME=/usr/lib/jvm/jdk-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.
Should we install the JRE instead? I suppose installing JDK instead of JRE would increase the image size. I'm not sure whether that increase in size is significant in relation to the overall image size or not. What are your thoughts about this?
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 would like, but in current 4.11 we are using the presence of shell to detect jdk11 and it is not present in the.
We should fix that problem, then we can use the jre.
Btw I don't know a good way to detect jdk11+
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.
@ravisharda here it is a fix to support JRE and only only JDK
#2436
@eolivelli based on the above conversations, I understand that this PR shall upgrade both Bookkeeper (to 4.11.0) and Java (to Java 11), and is it not desirable to separate the two. Pravega is now on Java 11, so the latter should be fine. As for Bookkeeper, it uses |
@ravisharda sure ! we have integration tests for very old clients |
@eolivelli The documentation at https://bookkeeper.apache.org/ mentions Is version |
Yes 4.11 can be considered "stable" from my point of view. We are working on a new website, and current one is outdated. We already released 4.10 "major" release, and 4.11 and we are going to release 4.12, so this is why I am telling that 4.9 is quite old, because it distant from the current active branches. |
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.
Descriptions of the changes in this PR: - Update "master" docker image to BK 4.11.0 - Update to OpenJDK11 the main Docker image and the Docker image used for integration tests - We are not using "JRE" because it does not bundle jshell and our script detects the presence of JDK11+ from the presence of file "jshell" Master Issue: #2387 Reviewers: Ravi Sharda <None>, Jia Zhai <[email protected]> This closes #2433 from eolivelli/fix/test-jdk11-upgrade-docker (cherry picked from commit 83dec6e) Signed-off-by: Enrico Olivelli <[email protected]>
Descriptions of the changes in this PR:
Master Issue: #2387