Skip to content
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

Modernize and consolidate JDKs usage across all stages of the build. Use JDK-17 as bundled JDK distribution to run tests #1358

Merged
merged 2 commits into from
Oct 13, 2021

Conversation

reta
Copy link
Collaborator

@reta reta commented Oct 12, 2021

Signed-off-by: Andriy Redko [email protected]

Description

Use JDK-17 as bundled JDK distribution to run tests. Tested on Windows / Linux / MacOS (x64), using Adoptium JDK distribution (https://adoptium.net/releases.html)

Issues Resolved

Part of #1351

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 04c4fe5bcee06a6965c4910f17befc03a32b7e68

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 04c4fe5bcee06a6965c4910f17befc03a32b7e68

…Use JDK-17 as bundled JDK distribution to run tests

Signed-off-by: Andriy Redko <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 95b28db

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 95b28db

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Precommit failure 04c4fe5bcee06a6965c4910f17befc03a32b7e68
Log 1303

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 95b28db

@dblock
Copy link
Member

dblock commented Oct 12, 2021

Let's get any other references to JDKs in READMEs fixed as well, please?

We also have things like https://github.com/opensearch-project/OpenSearch/blob/main/.ci/java-versions.properties

I'd also would like to discuss what the next steps are after we merge this. I think we need to drive a similar change into all plugins that release with OpenSearch.

We also had a ticket opened to lower the JDK used and bundled to 11. This goes the other way to 17. @reta, what do you suggest we want to do about that issue?

@nknize any concerns?

@reta
Copy link
Collaborator Author

reta commented Oct 12, 2021

Thanks for the review @dblock, I split the change into multiple tasks (#1351), this is just the first one to replace JDK-15 with JDK-17, I thought about getting rid of other JDKs / updating READMEs etc in the next PR, just to reduce the scope of change and simplify the review (btw, I have not found any mentions of JDK-15 in the docs).

We also had a ticket opened to lower the JDK used and bundled to 11. This goes the other way to 17. @reta, what do you suggest we want to do about that issue?

I could certainly explore this route, some build steps still require JDK-14 (bwc at least), I have not touched this part yet but next task / PR is supposed to be exactly about that.

@dblock
Copy link
Member

dblock commented Oct 13, 2021

Thanks for the review @dblock, I split the change into multiple tasks (#1351), this is just the first one to replace JDK-15 with JDK-17, I thought about getting rid of other JDKs / updating READMEs etc in the next PR, just to reduce the scope of change and simplify the review (btw, I have not found any mentions of JDK-15 in the docs).

👍

We also had a ticket opened to lower the JDK used and bundled to 11. This goes the other way to 17. @reta, what do you suggest we want to do about that issue?

I could certainly explore this route, some build steps still require JDK-14 (bwc at least), I have not touched this part yet but next task / PR is supposed to be exactly about that.

The switch for the bundled JDK from 15 to 17 is my only concern. I agree that 17 is more "right" than 15 since LTS, but it's maybe not what we want. Let's go ahead with this PR anyway and I am looking forward for the rest of this!

@dblock
Copy link
Member

dblock commented Oct 13, 2021

start gradle check

@reta
Copy link
Collaborator Author

reta commented Oct 13, 2021

@dblock thank you, already working on the next step.

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 95b28db
Log 677

Reports 677

@dblock
Copy link
Member

dblock commented Oct 13, 2021

❌   Gradle Check failure 95b28db Log 677

Reports 677

Doesn't look like a fluke, care to take a look?

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 5fab558

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 5fab558

@reta
Copy link
Collaborator Author

reta commented Oct 13, 2021

Doesn't look like a fluke, care to take a look?

The :distribution:tools:plugin-cli:test seems to be slow due to the secure random generator entropy source, I've added -Djava.security.egd=file:/dev/urandom to the JVM parameters. The clues coming from the thread dump below

   1) Thread[id=36, name=TEST-InstallPluginCommandTests.testOfficialPlatformPlugin-seed#[119935E5BBB84CD4], state=RUNNABLE, group=TGRP-InstallPluginCommandTests]
        at java.base@17/java.io.FileInputStream.readBytes(Native Method)
        at java.base@17/java.io.FileInputStream.read(FileInputStream.java:276)
        at java.base@17/java.io.FilterInputStream.read(FilterInputStream.java:132)
        at java.base@17/sun.security.provider.NativePRNG$RandomIO.readFully(NativePRNG.java:425)
        at java.base@17/sun.security.provider.NativePRNG$RandomIO.implGenerateSeed(NativePRNG.java:442)
        at java.base@17/sun.security.provider.NativePRNG.engineGenerateSeed(NativePRNG.java:227)
        at java.base@17/java.security.SecureRandom.generateSeed(SecureRandom.java:864)

The org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests.testReplicaThreadedThroughputDegradationAndRejection is unfortunately new flaky one, it fails on main as well, most of the time for me:

./gradlew ':server:test' --tests "org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests.testReplicaThreadedThroughputDegradationAndRejection" -Dtests.seed=8EC1ED900DC1D154 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1" -Dtests.locale=da -Dtests.timezone=America/Swift_Current 


=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 6.6.1
  OS Info               : Linux 5.11.0-37-generic (amd64)
  JDK Version           : 11 (JDK)
  JAVA_HOME             : /usr/lib/jvm/java-11-openjdk-amd64
  Random Testing Seed   : 8EC1ED900DC1D154
  In FIPS 140 mode      : false
=======================================


> Task :server:test

REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests.testReplicaThreadedThroughputDegradationAndRejection" -Dtests.seed=8EC1ED900DC1D154 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1" -Dtests.locale=da -Dtests.timezone=America/Swift_Current -Druntime.java=15

org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests > testReplicaThreadedThroughputDegradationAndRejection FAILED
    java.lang.AssertionError: expected:<1> but was:<8>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:645)
        at org.junit.Assert.assertEquals(Assert.java:631)
        at org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests.testReplicaThreadedThroughputDegradationAndRejection(ShardIndexingPressureConcurrentExecutionTests.java:506)

    com.carrotsearch.randomizedtesting.UncaughtExceptionError: Captured an uncaught exception in thread: Thread[id=114, name=Thread-97, state=RUNNABLE, group=TGRP-ShardIndexingPressureConcurrentExecutionTests]

....

Created #1361

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 5fab558

@dblock
Copy link
Member

dblock commented Oct 13, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 5fab558
Log 680

Reports 680

@reta
Copy link
Collaborator Author

reta commented Oct 13, 2021

x Gradle Check failure 5fab558 Log 680

Reports 680

Hm ... this is a new one,

Caused by: java.lang.IllegalArgumentException: duration cannot be negative, was given [-3002]
  1> 	at org.opensearch.common.unit.TimeValue.<init>(TimeValue.java:65) ~[opensearch-core-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
  1> 	at org.opensearch.common.unit.TimeValue.<init>(TimeValue.java:60) ~[opensearch-core-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
  1> 	at org.opensearch.test.transport.MockTransportService.lambda$addUnresponsiveRule$6(MockTransportService.java:350) ~[framework-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
  1> 	at org.opensearch.test.transport.MockTransportService$3.sendRequest(MockTransportService.java:392) ~[framework-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]

@dblock
Copy link
Member

dblock commented Oct 13, 2021

cc: @ryanbogan who's looking at #1276 I believe

@dblock
Copy link
Member

dblock commented Oct 13, 2021

start gradle check

@dblock
Copy link
Member

dblock commented Oct 13, 2021

x Gradle Check failure 5fab558 Log 680
Reports 680

Hm ... this is a new one,

Caused by: java.lang.IllegalArgumentException: duration cannot be negative, was given [-3002]
  1> 	at org.opensearch.common.unit.TimeValue.<init>(TimeValue.java:65) ~[opensearch-core-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
  1> 	at org.opensearch.common.unit.TimeValue.<init>(TimeValue.java:60) ~[opensearch-core-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
  1> 	at org.opensearch.test.transport.MockTransportService.lambda$addUnresponsiveRule$6(MockTransportService.java:350) ~[framework-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
  1> 	at org.opensearch.test.transport.MockTransportService$3.sendRequest(MockTransportService.java:392) ~[framework-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]

I kicked it again to see if it reproducible or whether we have solved time travel.

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 5fab558
Log 682

Reports 682

@dblock dblock merged commit 3779576 into opensearch-project:main Oct 13, 2021
@saratvemulapalli
Copy link
Member

Coming from #1910
@dblock / @reta curious why this was not backported to 1.x line?

@reta
Copy link
Collaborator Author

reta commented Jan 14, 2022

Coming from #1910 @dblock / @reta curious why this was not backported to 1.x line?

@saratvemulapalli I think #1358 (comment) and opensearch-project/opensearch-build#74 (comment) were the concerns from @dblock

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants