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

Update to Gradle 7.6 and JDK-19 #4973

Merged
merged 5 commits into from
Nov 8, 2022
Merged

Update to Gradle 7.6 and JDK-19 #4973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3e5e48f
Update to Gradle 7.6 and JDK-19
reta Oct 28, 2022
3f9c561
Address code review comments
reta Nov 8, 2022
0601206
Address code review comments
reta Nov 8, 2022
0167507
Update Gradle to 7.6-rc2
reta Nov 8, 2022
f276b0c
Removed unnecessary override
reta Nov 8, 2022
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- on-boarding of tasks([#4542](https://github.com/opensearch-project/OpenSearch/pull/4542))
- Integs ([4588](https://github.com/opensearch-project/OpenSearch/pull/4588))
- Integration tests for searchable snapshots ([#5048](https://github.com/opensearch-project/OpenSearch/pull/5048))
- Update to Gradle 7.6 and JDK-19 ([#4973](https://github.com/opensearch-project/OpenSearch/pull/4973))

### Dependencies
- Bumps `log4j-core` from 2.18.0 to 2.19.0
- Bumps `reactor-netty-http` from 1.0.18 to 1.0.23
Expand Down
4 changes: 4 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,10 @@ allprojects {
javadoc.options.encoding = 'UTF8'
javadoc.options.addStringOption('Xdoclint:all,-missing', '-quiet')
javadoc.options.tags = ["opensearch.internal", "opensearch.api", "opensearch.experimental"]
if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_19) {
javadoc.options.addBooleanOption("-enable-preview", true)
javadoc.options.addStringOption("-release", BuildParams.runtimeJavaVersion.majorVersion)
}
}

// support for reproducible builds
Expand Down
2 changes: 1 addition & 1 deletion buildSrc/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ dependencies {
api 'gradle.plugin.com.github.johnrengelman:shadow:7.1.2'
api 'org.jdom:jdom2:2.0.6.1'
api "org.jetbrains.kotlin:kotlin-stdlib-jdk8:${props.getProperty('kotlin')}"
api 'de.thetaphi:forbiddenapis:3.3'
api 'de.thetaphi:forbiddenapis:3.4'
api 'com.avast.gradle:gradle-docker-compose-plugin:0.15.2'
api "org.yaml:snakeyaml:${props.getProperty('snakeyaml')}"
api 'org.apache.maven:maven-model:3.6.2'
Expand Down
29 changes: 27 additions & 2 deletions buildSrc/src/main/java/org/opensearch/gradle/info/GlobalBuildInfoPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,15 @@
import org.gradle.internal.jvm.Jvm;
import org.gradle.internal.jvm.inspection.JvmInstallationMetadata;
import org.gradle.internal.jvm.inspection.JvmMetadataDetector;
import org.gradle.jvm.toolchain.internal.InstallationLocation;
import org.gradle.util.GradleVersion;

import javax.inject.Inject;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
Expand Down Expand Up @@ -195,7 +198,29 @@ private JavaVersion determineJavaVersion(String description, File javaHome, Java
}

private JvmInstallationMetadata getJavaInstallation(File javaHome) {
return jvmMetadataDetector.getMetadata(javaHome);
final InstallationLocation location = new InstallationLocation(javaHome, "Java home");

try {
try {
// The getMetadata(File) is used by Gradle pre-7.6
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:-(

Copy link
Member

Choose a reason for hiding this comment

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

Is it simpler to get the version of gradle and remove all these try/catches?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, but how Gradle version helps? We still need to call the method :-) Unless we set the minimum to 7.6 but in this case, it would be a "breaking" change for plugin developers - they would have to upgrade to Gradle 7.6 as well (similar pain of migration to 7.5)?

Copy link
Member

Choose a reason for hiding this comment

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

It's really NBD. I mean is there an if version of this code rather than relying on NoSuchMethodException or IllegalAccessException?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm ... the old refection mechanism could be used in such a way (get methods, try to find the one you need) but MethodHandles's methods do raise NoSuchMethodException sadly

return (JvmInstallationMetadata) MethodHandles.publicLookup()
.findVirtual(JvmMetadataDetector.class, "getMetadata", MethodType.methodType(JvmInstallationMetadata.class, File.class))
.bindTo(jvmMetadataDetector)
.invokeExact(location.getLocation());
} catch (NoSuchMethodException | IllegalAccessException ex) {
// The getMetadata(InstallationLocation) is used by Gradle post-7.6
return (JvmInstallationMetadata) MethodHandles.publicLookup()
.findVirtual(
JvmMetadataDetector.class,
"getMetadata",
MethodType.methodType(JvmInstallationMetadata.class, InstallationLocation.class)
)
.bindTo(jvmMetadataDetector)
.invokeExact(location);
}
} catch (Throwable ex) {
throw new IllegalStateException("Unable to find suitable JvmMetadataDetector::getMetadata", ex);
}
}

private List<JavaHome> getAvailableJavaVersions(JavaVersion minimumCompilerVersion) {
Expand All @@ -205,7 +230,7 @@ private List<JavaHome> getAvailableJavaVersions(JavaVersion minimumCompilerVersi
String javaHomeEnvVarName = getJavaHomeEnvVarName(Integer.toString(version));
if (System.getenv(javaHomeEnvVarName) != null) {
File javaHomeDirectory = new File(findJavaHome(Integer.toString(version)));
JvmInstallationMetadata javaInstallation = jvmMetadataDetector.getMetadata(javaHomeDirectory);
JvmInstallationMetadata javaInstallation = getJavaInstallation(javaHomeDirectory);
JavaHome javaHome = JavaHome.of(version, providers.provider(() -> {
int actualVersion = Integer.parseInt(javaInstallation.getLanguageVersion().getMajorVersion());
if (actualVersion != version) {
Expand Down
2 changes: 1 addition & 1 deletion buildSrc/src/main/java/org/opensearch/gradle/precommit/ThirdPartyAuditPrecommitPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public class ThirdPartyAuditPrecommitPlugin extends PrecommitPlugin {
public TaskProvider<? extends Task> createTask(Project project) {
project.getPlugins().apply(CompileOnlyResolvePlugin.class);
project.getConfigurations().create("forbiddenApisCliJar");
project.getDependencies().add("forbiddenApisCliJar", "de.thetaphi:forbiddenapis:3.2");
project.getDependencies().add("forbiddenApisCliJar", "de.thetaphi:forbiddenapis:3.4");

Configuration jdkJarHellConfig = project.getConfigurations().create(JDK_JAR_HELL_CONFIG_NAME);
if (BuildParams.isInternal() && project.getPath().equals(":libs:opensearch-core") == false) {
Expand Down
3 changes: 1 addition & 2 deletions buildSrc/version.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ opensearch = 3.0.0
lucene = 9.5.0-snapshot-a4ef70f

bundled_jdk_vendor = adoptium
bundled_jdk = 17.0.5+8

bundled_jdk = 19.0.1+10


# optional dependencies
Expand Down
4 changes: 2 additions & 2 deletions gradle/wrapper/gradle-wrapper.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-7.5.1-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-7.6-rc-2-all.zip
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionSha256Sum=db9c8211ed63f61f60292c69e80d89196f9eb36665e369e7f00ac4cc841c2219
distributionSha256Sum=be5387477162265eac882b9c83d756d8d2db170380e36fba2fdbee83d87de0d7
4 changes: 4 additions & 0 deletions test/framework/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ test {
systemProperty 'tests.gradle_index_compat_versions', BuildParams.bwcVersions.indexCompatible.join(',')
systemProperty 'tests.gradle_wire_compat_versions', BuildParams.bwcVersions.wireCompatible.join(',')
systemProperty 'tests.gradle_unreleased_versions', BuildParams.bwcVersions.unreleased.join(',')

if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_18) {
jvmArgs += ["-Djava.security.manager=allow"]
}
}

tasks.register("integTest", Test) {
Expand Down
39 changes: 22 additions & 17 deletions ...ork/src/main/java/org/opensearch/cluster/coordination/MockSinglePrioritizingExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,25 +44,30 @@
public class MockSinglePrioritizingExecutor extends PrioritizedOpenSearchThreadPoolExecutor {

public MockSinglePrioritizingExecutor(String name, DeterministicTaskQueue deterministicTaskQueue, ThreadPool threadPool) {
super(name, 0, 1, 0L, TimeUnit.MILLISECONDS, r -> new Thread() {
@Override
public void start() {
deterministicTaskQueue.scheduleNow(new Runnable() {
@Override
public void run() {
try {
r.run();
} catch (KillWorkerError kwe) {
// hacks everywhere
}
super(name, 0, 1, 0L, TimeUnit.MILLISECONDS, r -> {
// This executor used to override Thread::start method so the actual runnable is
// being scheduled in the scope of current thread of execution. In JDK-19, the Thread::start
// is not called anymore (https://bugs.openjdk.org/browse/JDK-8292027) and there is no
// suitable option to alter the executor's behavior in the similar way. The closest we
// could get to is to schedule the runnable once the ThreadFactory is being asked to
// allocate the new thread.
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a problem. How did you come up with a workaround and what do folks in https://bugs.openjdk.org/browse/JDK-8292027 think about it? Maybe add a comment there?

Copy link
Collaborator Author

@reta reta Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@dblock it does, and the fact the issue is still open and no alternative mechanism provided, it will be the probelm for quite some time.

I tried several approaches, the most straightforward one is to not override start and pass wrapped runnable, it turned out to be exceptionally problematic: from single threaded behaviour we are getting multi-threaded one (DeterministicTaskQueue is absolutely not suitable for that).

The idea behind this change: we know that the new thread is going to be allocated once thread pool decides that new worker is needed. We cannot override or alter the addWorker behaviour but we could "simulate" Thread::start at the moment new Thread is being created.

This implementation is laser targeted to the way MockSinglePrioritizingExecutor / DeterministicTaskQueue are used in the tests, I would not recommend it to be used anywhere else.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed explanation.

deterministicTaskQueue.scheduleNow(new Runnable() {
@Override
public void run() {
try {
r.run();
} catch (KillWorkerError kwe) {
// hacks everywhere
}
}

@Override
public String toString() {
return r.toString();
}
});
}
@Override
public String toString() {
return r.toString();
}
});

return new Thread(() -> {});
}, threadPool.getThreadContext(), threadPool.scheduler());
}

Expand Down