-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Add JDK11 support and enable in CI #31644
Conversation
Required to fix `bcprov-jdk15on-1.55.jar; invalid manifest format ` on jdk 11
To see if there are other failures on JDK11
Fix JAVA version to 10 for ES 6.3
``` > Task :distribution:bwc:next-bugfix-snapshot:buildBwcVersion [bwc] :buildSrc:compileJava [bwc] WARNING: An illegal reflective access operation has occurred [bwc] WARNING: Illegal reflective access by org.codehaus.groovy.reflection.CachedClass (file:/home/alpar/.gradle/wrapper/dists/gradle-4.5-all/cg9lyzfg3iwv6fa00os9gcgj4/gradle-4.5/lib/groovy-all-2.4.12.jar) to method java.lang.Object.finalize() [bwc] WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.reflection.CachedClass [bwc] WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations [bwc] WARNING: All illegal access operations will be denied in a future release [bwc] :buildSrc:compileGroovy [bwc] :buildSrc:writeVersionProperties [bwc] :buildSrc:processResources [bwc] :buildSrc:classes [bwc] :buildSrc:jar ```
So that we can make sure it's not too new for the build to understand.
Pinging @elastic/es-core-infra |
.ci/java-versions.properties
Outdated
@@ -4,5 +4,5 @@ | |||
# build and test Elasticsearch for this branch. Valid Java versions | |||
# are 'java' or 'openjdk' followed by the major release number. | |||
|
|||
ES_BUILD_JAVA=java10 | |||
ES_RUNTIME_JAVA=java8 | |||
ES_BUILD_JAVA=java11 |
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.
@jasontedor I'm actually not sure if this is ok, or we should have it only for matrix and do this lather on.
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.
Indeed, we need to hold off on this specific change and leave it to the matrix only for now until we reach conclusion on the minimum runtime JDK for 7.0.0.
I applied the v6.4.0 label, to back-port this PR -CI changes and muted tests. |
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 hate having all of the awaitsfixes but that is life I guess. Beyond those this looks good to me.
I suppose we assign those failing tests to some owner?
Can we not awaits fix those and instead make them assumeFalse(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.
I left one comment. I am not marking it as request changes because I will not be around next week to approve the change. I would lean on someone else to pick up this thread.
distribution/bwc/build.gradle
Outdated
} else if ("6.2".equals(bwcBranch)) { | ||
environment('JAVA_HOME', getJavaHome(it, 9)) | ||
environment('RUNTIME_JAVA_HOME', getJavaHome(it, 9)) |
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 is the reason for this change? I think it is because we can't run these builds when the run-time Java is JDK 11 and that is fine. If that's the case I think that we should lock these to the minimum supported JDK which is JDK 8.
@@ -295,6 +295,7 @@ public void testNonExistentWithoutIgnoreMissing() throws Exception { | |||
return attachmentData; | |||
} | |||
|
|||
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/31305") |
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 that we should use assumeFalse(JDK 11)
on these (use the JavaVersion.current().compareTo(JavaVersion.parse("11"))
that I pointed to before).
@@ -43,6 +43,7 @@ | |||
return pluginList(HdfsPlugin.class); | |||
} | |||
|
|||
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/31498") |
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 that we should use assumeFalse(JDK 11)
on these (use the JavaVersion.current().compareTo(JavaVersion.parse("11"))
that I pointed to before).
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 looks good. I left a few comments. Additionally, I think any issues that are linked here as TODOs need to be made blockers so that they are at least investigated and their impact assessed before 6.4.0.
@@ -88,7 +88,6 @@ java.lang.Thread#getAllStackTraces() | |||
|
|||
@defaultMessage Stopping threads explicitly leads to inconsistent states. Use interrupt() instead. | |||
java.lang.Thread#stop() | |||
java.lang.Thread#stop(java.lang.Throwable) |
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 removed from forbidden apis? The forbidden signatures should against our minimum runtime, which is still java 8.
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.
The method was removed in jdk 11 and causing the test to fail.
Do we have per JDK version forbidden APIs ?
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 not. I think this is related to running forbidden apis in a separate jvm. We should always be running it with the minimum supported version.
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.
Can you please comment this out and reference the issue to run forbidden apis in a forked jvm.
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.
Done, will remove comment once #31715 is fixed.
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.
If this issue was the only reason to no longer use the gradle plugin and fork instead, I can just say: The above forbidden-api signature can be removed anyways from elasticsearch (together with the previous one). Forbiddenapis has those signatures already in its "jdk-deprecated" ones, as it's a known problem - so there is no need to have it in Elasticsearch's custom signatures. "jdk-deprecated" automatically handles deprecations that were removed.
IMHO, #31715 was not a good idea, sorry!
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.
and
distribution/bwc/build.gradle
Outdated
@@ -196,3 +204,25 @@ subprojects { | |||
} | |||
} | |||
} | |||
|
|||
class IdentingOutputStream extends OutputStream { |
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: Identing -> Indenting
distribution/bwc/build.gradle
Outdated
public void write(int[] bytes, int offset, int length) { | ||
delegate.write( | ||
new String(bytes, offset, length) | ||
.replace("\n", "\n [bwc] ") |
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 this is going to be pretty inefficient. Instead, you can loop over the bytes looking for newline. When you find a newline, emit the array up to that point. Then emit a fixed byte array you have with the newline and prefix. Then continue the loop. You won't need to create any temporary objects.
* Upgrade bouncycastle Required to fix `bcprov-jdk15on-1.55.jar; invalid manifest format ` on jdk 11 * Downgrade bouncycastle to avoid invalid manifest * Add checksum for new jars * Update tika permissions for jdk 11 * Mute test failing on jdk 11 * Add JDK11 to CI * Thread#stop(Throwable) was removed http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-June/053536.html * Disable failing tests #31456 * Temprorarily disable doc tests To see if there are other failures on JDK11 * Only blacklist specific doc tests * Disable only failing tests in ingest attachment plugin * Mute failing HDFS tests #31498 * Mute failing lang-painless tests #31500 * Fix backwards compatability builds Fix JAVA version to 10 for ES 6.3 * Add 6.x to bwx -> java10 * Prefix out and err from buildBwcVersion for readability ``` > Task :distribution:bwc:next-bugfix-snapshot:buildBwcVersion [bwc] :buildSrc:compileJava [bwc] WARNING: An illegal reflective access operation has occurred [bwc] WARNING: Illegal reflective access by org.codehaus.groovy.reflection.CachedClass (file:/home/alpar/.gradle/wrapper/dists/gradle-4.5-all/cg9lyzfg3iwv6fa00os9gcgj4/gradle-4.5/lib/groovy-all-2.4.12.jar) to method java.lang.Object.finalize() [bwc] WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.reflection.CachedClass [bwc] WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations [bwc] WARNING: All illegal access operations will be denied in a future release [bwc] :buildSrc:compileGroovy [bwc] :buildSrc:writeVersionProperties [bwc] :buildSrc:processResources [bwc] :buildSrc:classes [bwc] :buildSrc:jar ``` * Also set RUNTIME_JAVA_HOME for bwcBuild So that we can make sure it's not too new for the build to understand. * Align bouncycastle dependency * fix painles array tets closes #31500 * Update jar checksums * Keep 8/10 runtime/compile untill consensus builds on 11 * Only skip failing tests if running on Java 11 * Failures are dependent of compile java version not runtime * Condition doc test exceptions on compiler java version as well * Disable hdfs tests based on runtime java * Set runtime java to minimum supported for bwc * PR review * Add comment with ticket for forbidden apis
* 6.x: Test: Do not remove xpack templates when cleaning (#31642) SQL: Allow long literals (#31777) SQL: Fix incorrect message for aliases (#31792) Detach Transport from TransportService (#31727) 6.3.1 release notes (#31829) Add unreleased version 6.3.2 [ML][TEST] Use java 11 valid time format in DataDescriptionTests (#31817) [ML] Don't treat stale FAILED jobs as OPENING in job allocation (#31800) [ML] Fix calendar and filter updates from non-master nodes (#31804) Fix license header generation on Windows (#31790) mark XPackRestIT.test {p0=monitoring/bulk/10_basic/Bulk indexing of monitoring data} as AwaitsFix Add JDK11 support without enabling in CI (#31644) Watcher: Fix check for currently executed watches (#31137) [DOCS] Fixes 6.3.0 release notes (#31771) Watcher: Ensure correct method is used to read secure settings (#31753) [ML] Rate limit established model memory updates (#31768) SQL: Update CLI logo
* master: REST high-level client: add get index API (#31703) SQL: Allow long literals (#31777) SQL: Fix incorrect message for aliases (#31792) Test: Do not remove xpack templates when cleaning (#31642) Reduce more raw types warnings (#31780) Add unreleased version 6.3.2 Scripting: Remove support for deprecated StoredScript contexts (#31394) [ML][TEST] Use java 11 valid time format in DataDescriptionTests (#31817) [ML] Don't treat stale FAILED jobs as OPENING in job allocation (#31800) [ML] Fix calendar and filter updates from non-master nodes (#31804) Fix license header generation on Windows (#31790) mark RollupIT.testTwoJobsStartStopDeleteOne as AwaitsFix mark SearchAsyncActionTests.testFanOutAndCollect as AwaitsFix Correct exclusion of test on JDK 11 Fix doclint jdk 11 Add JDK11 support and enable in CI (#31644) Watcher: Fix check for currently executed watches (#31137) Watcher: Ensure correct method is used to read secure settings (#31753) SQL: Update CLI logo
Closes #31230, #31500