-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Bump the minimum Java version to Java 11 #40754
Conversation
With the 8.0.0 release of Elasticsearch we will bump the minimum required Java to Java 11. This commit puts this into effect on the master branch.
Pinging @elastic/es-core-infra |
@jkakavas FYI this means that we will need a |
@danielmitterdorfer This will disrupt performance benchmarks, how do you want to proceed? |
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.
What I understand looks good, but please wait for @rjernst's review before pushing as I'm not very familiar with the build.
It looks like we could do minor simplifications in BuildPlugin.groovy
and PrecommitTasks.groovy
, which check for project.runtimeJavaVersion >= JavaVersion.VERSION_1_9
, which would always be true now, and project.compilerJavaVersion < JavaVersion.VERSION_1_10
which would always evaluate to false?
//tag::notable-breaking-changes[] | ||
==== Java 11 is required | ||
|
||
Java 11 is now required to run Elasticsearch and any of its command line tools. |
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.
do we need to say "11 or greater" to clarify that more recent versions are fine?
@@ -34,39 +34,39 @@ publishing { | |||
|
|||
archivesBaseName = 'elasticsearch' | |||
|
|||
// we want to keep the JDKs in our IDEs set to JDK 8 until minimum JDK is bumped to 9 so we do not include this source set in our IDEs | |||
// we want to keep the JDKs in our IDEs set to JDK 11 until minimum JDK is bumped to 17 so we do not include this source set in our IDEs |
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.
s/17/12/ ?
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 think it should be 18? The next LTS release?
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.
Yeah, I was going for the next LTS release which I had in my head was 17. I think that's right though? JDK LTS releases every three years, 11 was in September 2018 so the next one would be September 2021 which would be:
- 13 September 2019
- 14 March 2020
- 15 September 2020
- 16 March 2021
- 17 September 2021
configuredMaxHeapSize, mem, inputArguments, bootClassPath, classPath, systemProperties, gcCollectors, memoryPools, onError, | ||
onOutOfMemoryError, useCompressedOops, useG1GC, useSerialGC); | ||
INSTANCE = new JvmInfo( | ||
ProcessHandle.current().pid(), |
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.
note to other reviewers: this line is the thing that changed
Elasticsearch master bumps the minimum Java version to Java 11. With this commit we follow suit. Relates elastic/elasticsearch#40754
Elasticsearch master bumps the minimum Java version to Java 11. With this commit we follow suit. Relates elastic/elasticsearch#40754 Relates #25
Thanks for the ping; highly appreciated. We're all set from a benchmarking perspective and the cutover benchmarking run has just been triggered. From now on all our nightly benchmarks will run with OpenJDK 11.0.2. |
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.
Thanks! I left some comments and questions.
Note to self, there's some cleanup in buildSrc/build.gradle
that is made possible by this PR.
@@ -6,11 +6,8 @@ | |||
# or 'openjdk' followed by the major release number. | |||
|
|||
ES_RUNTIME_JAVA: | |||
- java8 | |||
- java8fips |
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.
Shouldn't we have an alternative before removing this?
There's #37250 so this will not fall trough the cracks, but in the interim we will have no fips testing,
and fips might have to play catch-up once we get the tests back.
I think that's something we should generally try to avoid in CI.
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 discussed this with @jkakavas and he will be working on getting a FIPS-compliant JDK 11 ready, but agreed that can happen independently. He will focus on that in the upcoming week.
@@ -246,6 +246,10 @@ def linux_common(config, | |||
touch /is_vagrant_vm # for consistency between linux and windows | |||
SHELL | |||
|
|||
config.vm.provision 'jdk-11', type: 'shell', inline: <<-SHELL | |||
curl -sSL https://download.java.net/java/GA/jdk11/9/GPL/openjdk-11.0.2_linux-x64_bin.tar.gz | tar xz -C /opt/ |
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 can potentially lead to more CI failures due to network issues.
On the other hand I'm not sure we need to pull in any JDK for the packaging tests,
AFAIK we test with the java version from the platform under test, if this is to old we should just stick to the bundled one or have the set set JAVA_HOME to that location of that's what we want to test.
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 need a non-bundled JDK in the image that can run Elasticsearch to test that a non-bundled JDK works. The installed JDK 8s would be too old, so we need JDK 11.
@@ -34,39 +34,39 @@ publishing { | |||
|
|||
archivesBaseName = 'elasticsearch' | |||
|
|||
// we want to keep the JDKs in our IDEs set to JDK 8 until minimum JDK is bumped to 9 so we do not include this source set in our IDEs | |||
// we want to keep the JDKs in our IDEs set to JDK 11 until minimum JDK is bumped to 17 so we do not include this source set in our IDEs |
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.
Why aren't we removing the additional source set just like we did for libs ?
Are we anticipating a need to to MR jars for java 12 ?
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.
Yeah, I'm anticipating that indeed.
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
Vagrantfile
Outdated
fi | ||
export SYSTEM_JAVA_HOME=\\\$JAVA_HOME | ||
unset JAVA_HOME | ||
export PATH=/opt/jdk-11.0.2/bin:\\\$PATH |
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.
Why is this added to path? The packaging tests don't use java on path anymore.
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 do use Java on the path. We have lines like:
local java=$(which java)
I'll rework these tests so that they aren't using Java from the path.
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.
Here is one example:
elasticsearch/qa/vagrant/src/test/resources/packaging/tests/module_and_plugin_test_cases.bash
Line 433 in fef2153
local java=$(which java) |
@@ -34,39 +34,39 @@ publishing { | |||
|
|||
archivesBaseName = 'elasticsearch' | |||
|
|||
// we want to keep the JDKs in our IDEs set to JDK 8 until minimum JDK is bumped to 9 so we do not include this source set in our IDEs | |||
// we want to keep the JDKs in our IDEs set to JDK 11 until minimum JDK is bumped to 17 so we do not include this source set in our IDEs |
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 think it should be 18? The next LTS release?
* master: (77 commits) Suppress lease background sync failures if stopping (elastic#40902) [DOCS] Added settings page for ILM. (elastic#40880) [Docs] Remove extraneous text (elastic#40914) Move test classes to test root in Painless (elastic#40873) Fix date index name processor default date_formats (elastic#40915) Source additional files correctly in elasticsearch-cli (elastic#40890) Allow AVX-512 on JDK 11+ (elastic#40828) [Docs] Change example to show col headers (elastic#40822) Update apache httpclient to version 4.5.8 (elastic#40875) Update monitoring-kibana.json (elastic#40899) Introduce Delegating ActionListener Wrappers (elastic#40129) Deprecate old transport settings (elastic#40821) Add Kibana application privileges for monitoring and ml reserved roles (elastic#40651) Use Writeable for TransportReplAction derivatives (elastic#40894) Add test for HTTP and Transport TLS on basic license (elastic#40714) Remove unneded cluster config from test (elastic#40856) Make Fuzziness reject illegal values earlier (elastic#33511) Remove test-only customisation from TransReplAct (elastic#40863) Fix dense/sparse vector limit documentation (elastic#40852) Make -try xlint warning disabled by default. (elastic#40833) ...
@elasticmachine run elasticsearch-ci/2 |
* elastic/master: Fix Failing to Handle Ex. in TransportShardBulkAction (elastic#40923) Be lenient when parsing build flavor and type on the wire (elastic#40734) Make Transport Shard Bulk Action Async (elastic#39793)
* elastic/master: Mute failing IndexShard local history test
With the 8.0.0 release of Elasticsearch we will bump the minimum required Java to Java 11. This commit puts this into effect on the master branch.
The logger usage check uses its own version of ASM to inspect class files for logging usages. Master was updated to support java 11 compilation in #40754. However, 7.x still used ASM 5, which could not read newer java bytecode versions. This commit bumps ASM in 7.x used in the logger usage check. closes #52408
The logger usage check uses its own version of ASM to inspect class files for logging usages. Master was updated to support java 11 compilation in #40754. However, 7.x still used ASM 5, which could not read newer java bytecode versions. This commit bumps ASM in 7.x used in the logger usage check. closes #52408
With the 8.0.0 release of Elasticsearch we will bump the minimum required Java to Java 11. This commit puts this into effect on the master branch.