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.

Sign up for GitHub

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. Update JDK-14 requirement, switch to JDK-17 instead #1368

Merged
merged 4 commits into from
Oct 15, 2021
Merged

Modernize and consolidate JDKs usage across all stages of the build. Update JDK-14 requirement, switch to JDK-17 instead #1368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e18dbd8
Modernize and consolidate JDKs usage across all stages of the build. …
reta Oct 14, 2021
f55b0ea
Updating phrasing based on review feedback
reta Oct 14, 2021
fe6157f
Fixed runtime Java version usage, it has to be respected when RUNTIME…
reta Oct 14, 2021
4349dbf
Addressing review comments
reta Oct 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions DEVELOPER_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ Fork [opensearch-project/OpenSearch](https://github.com/opensearch-project/OpenS

OpenSearch builds using Java 11 at a minimum. This means you must have a JDK 11 installed with the environment variable `JAVA_HOME` referencing the path to Java home for your JDK 11 installation, e.g. `JAVA_HOME=/usr/lib/jvm/jdk-11`.

By default, tests use the same runtime as `JAVA_HOME`. However, since OpenSearch also supports JDK 8 as the runtime, the build supports compiling with JDK 11 and testing on a different version of JDK runtime. To do this, set `RUNTIME_JAVA_HOME` pointing to the Java home of another JDK installation, e.g. `RUNTIME_JAVA_HOME=/usr/lib/jvm/jdk-8`.
By default, the test tasks use bundled JDK runtime, configured in `buildSrc/version.properties` and set to JDK 17 (LTS). Other kind of test tasks (integration, cluster, ... ) use the same runtime as `JAVA_HOME`. However, since OpenSearch supports JDK 8/17 as the runtime, the build supports compiling with JDK 11 and testing on a different version of JDK runtime. To do this, set `RUNTIME_JAVA_HOME` pointing to the Java home of another JDK installation, e.g. `RUNTIME_JAVA_HOME=/usr/lib/jvm/jdk-8`. Alernatively, the runtime JDK version could be provided as the command line argument, using combination of `runtime.java=<major JDK version>` property and `JAVA<major JDK version>_HOME` environment variable, for exampe `./gradlew -Druntime.java=17 ...` (in this case, the tooling expects `JAVA17_HOME` environment variable to be set).
Copy link
Collaborator

@tlfeng tlfeng Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Andriy, a little concern regarding the expression "OpenSearch supports JDK 8/17 as the runtime". 😄
My concern is if listing 2 JDK versions, will it mislead the readers that only the 2 versions are supported? Actually other versions are able to use as runtime.
In my opinion, the original expression "However, since OpenSearch supports JDK 8 as the runtime" aims to emphasize the lowest JDK version for the runtime.
It's fine if there is actually no ambiguity on this expression.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely agree, changing the phrasing, thank you

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! While the original expression might also have a little confusion - whether only JDK 8 is supported as the runtime. 😂 Haha, anyway, a good solution might be to put the supporting matrix of OpenSearch and JVM in somewhere in user's documentation, but it's out of scope.
Glad to see you improving the developer guide!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tlfeng , I think the build matrix is the goal, hopefully we will get there soon, discussion is ongoing in opensearch-project/opensearch-build#732

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for providing the link 😄 (seems there are many GitHub issues regarding the JDK versions). I will keep an eye on the discussion.


To run the full suite of tests you will also need `JAVA8_HOME`, `JAVA11_HOME`, and `JAVA14_HOME`. They are required by the [backwards compatibility test](./TESTING.md#testing-backwards-compatibility).
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JAVA14_HOME is still needed, we checkout 1.x/1.0 branches and they do reference JDK-14

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, JAVA14_HOME is only used for building OpenSearch 1.0, it's a compromise for the smooth transition with Elasticsearch 7.10.2 🙂.


Expand All @@ -70,8 +70,8 @@ Start by running the test suite with `gradlew check`. This should complete witho
OpenSearch Build Hamster says Hello!
Gradle Version : 6.6.1
OS Info : Linux 5.4.0-1037-aws (amd64)
JDK Version : 14 (JDK)
JAVA_HOME : /usr/lib/jvm/java-14-openjdk-amd64
JDK Version : 11 (JDK)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we lowered it to 11

JAVA_HOME : /usr/lib/jvm/java-11-openjdk-amd64
=======================================

...
Expand Down Expand Up @@ -133,7 +133,7 @@ Use `-Dtests.opensearch.` to pass additional settings to the running instance. F

### IntelliJ IDEA

When importing into IntelliJ you will need to define an appropriate JDK. The convention is that **this SDK should be named "14"**, and the project import will detect it automatically. For more details on defining an SDK in IntelliJ please refer to [this documentation](https://www.jetbrains.com/help/idea/sdk.html#define-sdk). Note that SDK definitions are global, so you can add the JDK from any project, or after project import. Importing with a missing JDK will still work, IntelliJ will report a problem and will refuse to build until resolved.
When importing into IntelliJ you will need to define an appropriate JDK. The convention is that **this SDK should be named "11"**, and the project import will detect it automatically. For more details on defining an SDK in IntelliJ please refer to [this documentation](https://www.jetbrains.com/help/idea/sdk.html#define-sdk). Note that SDK definitions are global, so you can add the JDK from any project, or after project import. Importing with a missing JDK will still work, IntelliJ will report a problem and will refuse to build until resolved.

You can import the OpenSearch project into IntelliJ IDEA as follows.

Expand Down
4 changes: 2 additions & 2 deletions ...est/resources/org/opensearch/gradle/internal/fake_git/remote/.ci/java-versions.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,6 @@
# specific language governing permissions and limitations
# under the License.
#
OPENSEARCH_BUILD_JAVA=openjdk14
OPENSEARCH_RUNTIME_JAVA=openjdk14
OPENSEARCH_BUILD_JAVA=openjdk17
Copy link
Collaborator Author

@reta reta Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dblock should we lower it to openjdk11 and runtime to 8 like .ci/java-versions.properties?

OPENSEARCH_BUILD_JAVA=openjdk11
OPENSEARCH_RUNTIME_JAVA=java8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

OPENSEARCH_RUNTIME_JAVA=openjdk17
GRADLE_TASK=build
4 changes: 2 additions & 2 deletions buildSrc/src/main/java/org/opensearch/gradle/test/DistroTestPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@
public class DistroTestPlugin implements Plugin<Project> {
private static final String SYSTEM_JDK_VERSION = "8u242+b08";
private static final String SYSTEM_JDK_VENDOR = "adoptopenjdk";
private static final String GRADLE_JDK_VERSION = "14+36@076bab302c7b4508975440c56f6cc26a";
private static final String GRADLE_JDK_VENDOR = "openjdk";
private static final String GRADLE_JDK_VERSION = "17+35";
private static final String GRADLE_JDK_VENDOR = "adoptium";

// all distributions used by distro tests. this is temporary until tests are per distribution
private static final String EXAMPLE_PLUGIN_CONFIGURATION = "examplePlugin";
Expand Down