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

[DRAFT] Build this project and unit test using JDK 8. #157

Closed
wants to merge 1 commit into from
Closed

[DRAFT] Build this project and unit test using JDK 8. #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion DEVELOPER_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ Fork [opensearch-project/opensearch-java](https://github.com/opensearch-project/

### Install Prerequisites

To run the full suite of tests, download and install [JDK 14](https://jdk.java.net/archive/). Any JDK >= 11 works.
To run the full suite of tests, download and install [JDK 11](https://jdk.java.net/archive/). Any JDK >= 11 works.

The build will additionally use a JDK 8 to compile the library. You may install your own SDK, or Gradle will install one.

#### Docker

Expand Down
2 changes: 1 addition & 1 deletion config/checkstyle/checkstyle_suppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,5 @@
<suppress files="client[/\\]src[/\\]main[/\\]java[/\\]org[/\\]opensearch[/\\]client[/\\]opensearch[^.]*\.java" checks="UnusedImports" />
<suppress files="client[/\\]src[/\\]main[/\\]java[/\\]org[/\\]opensearch[/\\]client[/\\]opensearch[/\\]OpenSearch[^.]*\.java" checks="LineLength" />
<suppress files="client[/\\]src[/\\]main[/\\]java[/\\]org[/\\]opensearch[/\\]client[/\\]opensearch[/\\]OpenSearch[^.]*\.java" checks="UnusedImports" />
<suppress files="client[/\\]src[/\\]test[/\\]java[/\\]org[/\\]opensearch[/\\]client[/\\]opensearch[/\\]integTest[^.]*\.java" checks="Header" />
<suppress files="client[/\\]src[/\\]integrationTest[/\\]java[/\\]org[/\\]opensearch[/\\]client[/\\]opensearch[/\\]integTest[^.]*\.java" checks="Header" />
</suppressions>
49 changes: 39 additions & 10 deletions java-client/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,9 @@ checkstyle {
}

java {
targetCompatibility = JavaVersion.VERSION_11
sourceCompatibility = JavaVersion.VERSION_11
toolchain {
languageVersion.set(JavaLanguageVersion.of(8))
}

withJavadocJar()
withSourcesJar()
Expand Down Expand Up @@ -94,6 +95,28 @@ tasks.withType<Jar> {
}
}

sourceSets {
create("integrationTest") {
compileClasspath += sourceSets.main.get().output + sourceSets.test.get().output
runtimeClasspath += sourceSets.main.get().output + sourceSets.test.get().output
java {
setSrcDirs(listOf("src/integrationTest/java"))
}
}
}

val integrationTestImplementation by configurations.getting {
extendsFrom(configurations.testImplementation.get())
}

configurations["integrationTestRuntimeOnly"].extendsFrom(configurations.runtimeOnly.get())

tasks.named<JavaCompile>("compileIntegrationTestJava").configure {
javaCompiler.set(javaToolchains.compilerFor {
languageVersion.set(JavaLanguageVersion.of(11))
})
}

tasks.test {
systemProperty("tests.security.manager", "false")

Expand All @@ -105,15 +128,20 @@ tasks.test {
}

val unitTest = task<Test>("unitTest") {
filter {
excludeTestsMatching("org.opensearch.client.opensearch.integTest.*")
}
}

val integrationTest = task<Test>("integrationTest") {
filter {
includeTestsMatching("org.opensearch.client.opensearch.integTest.*")
}
description = "Runs integration tests."
group = "verification"

testClassesDirs = sourceSets["integrationTest"].output.classesDirs
classpath = sourceSets["integrationTest"].runtimeClasspath
shouldRunAfter("test")

javaLauncher.set(javaToolchains.launcherFor {
languageVersion.set(JavaLanguageVersion.of(11))
})

systemProperty("tests.security.manager", "false")
// Basic auth settings for integration test
systemProperty("https", System.getProperty("https", "true"))
Expand All @@ -128,8 +156,8 @@ dependencies {
val jacksonDatabindVersion = "2.12.6.1"

// Apache 2.0
implementation("org.opensearch.client", "opensearch-rest-client", opensearchVersion)
testImplementation("org.opensearch.test", "framework", opensearchVersion)
implementation("org.opensearch.client", "opensearch-rest-client", "1.3.2")
integrationTestImplementation("org.opensearch.test", "framework", opensearchVersion)
Copy link
Collaborator

@reta reta May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If anything, we should not take this path: opensearch-rest-client of one version, framework of another version, just my opinion. The clean solution would be to have: a) concise Opensearch version b) clean build under 8 and 11 toolchains.

Copy link
Member Author

Choose a reason for hiding this comment

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

@reta , I mentioned the 1.3.2 problem in the PR description. And I fully agree that this is not the right approach - it is temporary to have a working build. I aim to be able to remove with opensearch-project/OpenSearch#3181. But, without that merged in, this would not work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, thanks @dlvenable , I still don't understand how it is supposed to work with framework (the opensearch-rest-client will be fine): the JDK-8 toolchain will fail trying to use JDK-11 bytecode.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR builds and runs unit tests against JDK 8. It runs the integration tests using JDK 11 to support the framework library.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Uh ... thanks @dlvenable , @dblock supporting this JDK-8/JDK-11 "split brain" across project would be quite tricky, we need a really compelling reason to go with this change (#156 has no evidence it is really needed, @dlvenable please correct me if I am wrong).

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I do think this is a mess, and OpenSearch is exposing this mess to Java clients. I'd like to break that somehow.

Copy link
Member

Choose a reason for hiding this comment

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

That's opensearch-project/opensearch-clients#19 !

Copy link
Contributor

Choose a reason for hiding this comment

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

The low-level REST client provides very little value to this library. I suggest you just git rid of it and use its underlying HTTP transport instead. One less dependency to worry about.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Maybe you want to take a stab at that @mtimmerm? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe. I'll look into it

implementation("org.apache.logging.log4j", "log4j-core", "2.17.2")

// Apache 2.0
Expand Down Expand Up @@ -158,6 +186,7 @@ dependencies {
// EPL-2.0 OR BSD-3-Clause
// https://eclipse-ee4j.github.io/yasson/
implementation("org.eclipse", "yasson", "2.0.2")
testImplementation("org.hamcrest:hamcrest:2.1")

// https://github.com/classgraph/classgraph
testImplementation("io.github.classgraph:classgraph:4.8.116")
Expand Down
0 ...opensearch/integTest/ClusterClientIT.java → ...opensearch/integTest/ClusterClientIT.java
View file
Open in desktop
File renamed without changes.
0 ...h/client/opensearch/integTest/CrudIT.java → ...h/client/opensearch/integTest/CrudIT.java
View file
Open in desktop
File renamed without changes.
0 ...opensearch/integTest/IndicesClientIT.java → ...opensearch/integTest/IndicesClientIT.java
View file
Open in desktop
File renamed without changes.
0 ...penSearchRestHighLevelClientTestCase.java → ...penSearchRestHighLevelClientTestCase.java
View file
Open in desktop
File renamed without changes.
0 ...t/opensearch/integTest/PingAndInfoIT.java → ...t/opensearch/integTest/PingAndInfoIT.java
View file
Open in desktop
File renamed without changes.
0 ...ent/opensearch/integTest/RequestTest.java → ...ent/opensearch/integTest/RequestTest.java
View file
Open in desktop
File renamed without changes.
14 changes: 12 additions & 2 deletions java-client/src/test/java/org/opensearch/client/opensearch/json/JsonpMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,9 @@
import jakarta.json.stream.JsonParser;
import org.junit.Assert;
import org.junit.Test;
import org.opensearch.core.internal.io.IOUtils;

import java.io.Closeable;
import java.io.IOException;
import java.io.StringReader;
import java.io.StringWriter;
import java.io.Writer;
Expand Down Expand Up @@ -113,7 +114,7 @@ public void testJacksonCustomJsonFactory() {
}

testDeserialize(mapper, writer.toString());
IOUtils.closeWhileHandlingException(writer);
closeWhileHandlingException(writer);
}

private void testSerialize(JsonpMapper mapper, String expected) {
Expand Down Expand Up @@ -196,4 +197,13 @@ public void setChildren(List<SomeClass> children) {
this.children = children;
}
}

private static void closeWhileHandlingException(final Closeable closeable) {
// noinspection EmptyCatchBlock
try {
if (closeable != null) {
closeable.close();
}
} catch (final IOException | RuntimeException e) {}
}
}