-
Notifications
You must be signed in to change notification settings - Fork 674
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
Bump up Java version to 21 #2682
Conversation
This happens some time! https://github.com/apache/solr/actions/runs/10627729009/job/29461325476?pr=2682
|
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.
BTW we don't have to compile with JDK 17 or use JDK 17 at all for the build. We just need to tell JDK 21 that a few modules for SolrJ are to be compiled with Java 17 language / support, allowing users on JDK 17 to use it. I don't recall the particulars of doing this.
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java
Outdated
Show resolved
Hide resolved
|
Note that "api" module should also be Java Language 17, as it's included with SolrJ. |
Looks like below config is working. It sets solrj and related projects to Jdk17 and rest of the projects to Jdk21 for main. And, for all the test packages they are using Jdk21. javac.gradle
Now, to test it. I unpack the builds and verify it in MANIFEST.MF. solrj
api
solr-zookeeper
solrj-streaming
core
But I would really appreciate If someone else test it and check If we able to achieve the different version for solr (Jdk21) and solrj(jdk17). |
Nice work. I toyed with it a bit, including tinkering with toolchains. Avoiding toolchains avoids IntelliJ's Gradle's check to tell you that we really should be using a newer Gradle Version:
|
Why aren't we using toolchains?: https://docs.gradle.org/current/userguide/toolchains.html |
The Docker build.gradle needs to refer to a JDK 21 base image, not JDK 17. BTW, when I checked out your branch and before I tinkered with it, I simply tried to get IntelliJ to sync it and then view IntelliJ's module language versions to see if it would get confused on the SolrJ modules, given differing desired language levels for tests. It was confused; it thought the whole module should use JDK 17, even the test sourcepath. I suspect the issue is that we don't use proper metadata (i.e. toolchain) to convey to IntelliJ what's going on. |
The toolchain cannot be set at the JavaCompile level. Because we are using different Jdk for main and test package. To set at project level, below is the config. But this fails!
Next, we have different toolchain for the tasks. And below is the way to configure different toolchain for compileJava and compileTestJava.
This works! I ran with
However, jar task is still, I think ,running with whatever java_home isBecause whenever I open MANIFEST.MF, Build-JDK is 21. I mean we are successfully able to compile files using jdk 17, however whenever jar task is running it's running via 21.
I try to add the toolchain config to
|
When it comes to Intellij module language level, I am not sure How we will solve that? Given that all solrj and realted modules have two jdk versions now. |
We use Toolchain to enforce the specific version of the tools we want to use. Here are some use cases:
In our scenario, if the system is configured with JDK 21, that version will be used to run all tasks for Solr and SolrJ. JDK 21 can generate class files with version 61 without issues. However, if we want SolrJ to compile only with JDK 17, Toolchain can accomplish that. Otherwise, the Java version used by Gradle will be applied globally. |
From Intellij docs: https://www.jetbrains.com/help/idea/gradle.html#gradle_jvm
|
IMO, There is no way to configure toolchain for jar task like we can do for JavaCompile. Even the JarTask has no mention of compiler like JavaCompiler has. https://github.com/gradle/gradle/blob/master/platforms/jvm/platform-jvm/src/main/java/org/gradle/jvm/tasks/Jar.java My question is that does it even matter which jdk version actually package the class files? Given that we made sure that compileJava task ran only by appropriate jdk. |
It's normal & acceptable that the entire build itself do JAR packaging etc with the same running JVM, that which Gradle is using -- JDK 21. We shouldn't try to change that or validate the manifest entry for the build JDK. It's just some FYI metadata. RE IntelliJ -- lots of us use IntelliJ; if IntelliJ will not tolerate Java 21 language features in a SolrJ test, we'll have to use Java 17 for those tests. Okay, not a big deal. |
gradle Auto-provisioning is disabled.
Build is failing due to that. It's like now we are basically forcing the version of tools that being used, in this case two Jdk version 17 and 21. Otherwise, If you have both version available then no issues.
There is a way to enable it via gradle.properties. (I have to check how I can make this file change on crave, it's not picking it up.)
There is somewhat related discussion regarding the same in alternative-jdk-support.gradle. solr/gradle/testing/alternative-jdk-support.gradle Lines 25 to 35 in a4cc580
|
I might have misunderstood Toolchains; if it means having additional JDKs around then I don't think we want to require that. It's sufficient to have only JDK 21 and configure the language level for certain parts. |
As I mentioned, IntelliJ isn't recognizing the language distinction between tests vs non-tests so I recommend simply using the same language per module. Other than that, I suppose it's ready to merge. At some point (could be now), we should review all the extra options we pass to java when starting Solr to see if they are still sensible. Some may be obsolete. |
5c721f3
to
ea75378
Compare
|
solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc
Outdated
Show resolved
Hide resolved
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.
LGTM. I love seeing lots of "workarounds" going away! Java21 is going to let us move to the latest OpenNLP and do some cool things with ONNX for ML models..
https://issues.apache.org/jira/browse/SOLR-17321
Bump up Java version to 21. Although we have decided to have different versions for Solr and SolrJ to 21 and 17 respectively. Currently in this PR, both of them are pointing to 21 to seek feedback and to test everything. Meanwhile looking for way to set SolrJ to 17.