solr icon indicating copy to clipboard operation
solr copied to clipboard

SOLR-17321: Bump minimum required Java version to 21

Open iamsanjay opened this issue 1 year ago • 9 comments

https://issues.apache.org/jira/browse/SOLR-17321

We are upgrading the minimum Java version for Solr main branch to 21. However, at the same, It has been suggested to be not so aggressive with SolrJ (and thus solr-api, a dependency) Java version – setting it to 17.

Depreciated API

  1. AccessController [Deprecated since 17 and marked for removal]
  2. AccessControlContext [Deprecated since 17 and marked for removal]
  3. java.net.URL ctors [Deprecated since 20]
  4. SecurityManager [Deprecated since 17 and marked for removal]
  5. Thread#getId() [Deprecated in Java 19]

To not make any extra code changes below annotation were used:

 @SuppressForbidden(reason = "<custom message>") //Still need to decide what the message should be for all. Thinking of adding "Deprecated in Java <version>"

@SuppressWarnings("removal")

However, at few places, for demonstration purpose, URL has been created via new suggested way instead of using the ctors:

url = new URL(urlStr);
url = URI.create(urlStr).toURL();

Or,

URL solrURL = new URI(solrUrl).toURL();

Also, Bump up the ecj version to 3.37 and ca.cutterslade.analyze to 1.9.2.

Checklist

Please review the following and check all that apply:

  • [x] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • [x] I have created a Jira issue and added the issue ID to my pull request title.
  • [x] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • [x] I have developed this patch against the main branch.
  • [x] I have run ./gradlew check.
  • [] I have added tests for my changes.
  • [ ] I have added documentation for the Reference Guide

iamsanjay avatar Jun 04 '24 16:06 iamsanjay

For now two things pending

  1. Set different Java level for SolrJ and Solr. Currently both are 21.
  2. And, another one is to change the test-via-crave.yml file to run the build with Java 21.

Feel free to push changes to this branch.

iamsanjay avatar Jun 04 '24 16:06 iamsanjay

@SuppressForbidden(reason = "") //Still need to decide what the message should be for all. Thinking of adding "Deprecated in Java "

Ideally those are addressed for real, otherwise they will continue to linger. Could be a separate JIRA/PR.

dsmiley avatar Jun 04 '24 19:06 dsmiley

RE URL: If you have IntelliJ Ultimate edition (not sure if Community), you could do "Structural Replace" to find & replace systematically everywhere.

Like always, Thanks for the air support. "Structural Replace" is a Game Changer!

iamsanjay avatar Jun 05 '24 12:06 iamsanjay

RE URL: If you have IntelliJ Ultimate edition (not sure if Community), you could do "Structural Replace" to find & replace systematically everywhere.

Like always, Thanks for the air support. "Structural Replace" is a Game Changer!

I need to learn that! Looks amazing!

epugh avatar Jun 05 '24 12:06 epugh

Would it be too much to ask to separate all this URL conversion stuff to a separate PR, ultimately being its own commit focused on exactly that? Needn't be another JIRA; needn't touch CHANGES.txt in that commit. If I had to do this, I'd start with the trick of taking a GitHub PR URL and adding the ".patch" (or maybe it's ".diff" for a different format) and then copy-paste into IntellIJ "Apply patch from Clipboard" and selectively exclude stuff unrelated at that point (or after).

So SuppressForbidden is okay for now, and later on (Just after this one!) deal with removing it in a different PR?

iamsanjay avatar Jun 05 '24 12:06 iamsanjay

I suggest flipping the order. Prep the code for Java 21 compatibility first so that we don't go adding SupressForbidden to many classes to only then fix the issue later. i.e. visit these classes once not twice. You know exactly how to fix it and have even starting doing so; it's not a tricky matter. Again, IntelliJ will do much of the work.

On Wed, Jun 5, 2024 at 8:55 AM Sanjay Dutt @.***> wrote:

Would it be too much to ask to separate all this URL conversion stuff to a separate PR, ultimately being its own commit focused on exactly that? Needn't be another JIRA; needn't touch CHANGES.txt in that commit. If I had to do this, I'd start with the trick of taking a GitHub PR URL and adding the ".patch" (or maybe it's ".diff" for a different format) and then copy-paste into IntellIJ "Apply patch from Clipboard" and selectively exclude stuff unrelated at that point (or after).

So SuppressForbidden is okay for now, and later on (Just after this one!) deal with removing it in a different PR?

— Reply to this email directly, view it on GitHub https://github.com/apache/solr/pull/2497#issuecomment-2149791390, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC4DTZJTS2ICJN2TM6AJZLZF4C77AVCNFSM6AAAAABIY26TRCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBZG44TCMZZGA . You are receiving this because your review was requested.Message ID: @.***>

dsmiley avatar Jun 05 '24 13:06 dsmiley

Test case failing, on main branch as well!

./gradlew :solr:core:test --tests "org.apache.solr.search.function.TestDenseVectorFunctionQuery.floatFieldVectors_shouldReturnFloatSimilarity" -Ptests.jvms=96 "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=8F0CED8E85B304BD -Ptests.timeoutSuite=600000! -Ptests.file.encoding=US-ASCII

iamsanjay avatar Jun 05 '24 15:06 iamsanjay

References to JavaVersion (a Gradle class) like >= JavaVersion.VERSION_16 should be reviewed to see if we can make changes to remove conditions that will no longer vary.

Searching for "Java 17" or "JDK 17" or similar searches or other versions can reveal stuff where maybe a change is needed.

dsmiley avatar Jun 05 '24 16:06 dsmiley

Setting different Java version for main and SolrJ resulting in error. Currently, :solr:api:ecjLintTest task is failing and below is the error.

> Task :solr:api:ecjLintTest
----------
1. ERROR in /Users/sanjaydutt/Documents/Solr/solr-main/solr/solr/api/src/test/org/apache/solr/util/TestSolrVersion.java (at line 19)
        import org.apache.solr.SolrTestCase;
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The import org.apache.solr.SolrTestCase cannot be resolved
----------
2. ERROR in /Users/sanjaydutt/Documents/Solr/solr-main/solr/solr/api/src/test/org/apache/solr/util/TestSolrVersion.java (at line 23)
        public class TestSolrVersion extends SolrTestCase {
                                             ^^^^^^^^^^^^
SolrTestCase cannot be resolved to a type

The api test cases not able to access the class, SolrTestClass, from the main branch. Further, I logged the java version that has been used to configure SolrJ via adding println statements.

ecj-lint.gradle

allprojects {
  plugins.withType(JavaPlugin) {
    // Create a [sourceSetName]EcjLint task for each source set
    // with a non-empty java.srcDirs. These tasks are then
    // attached to project's "ecjLint" task.

    println "The project name is " + project + " and the sourceCompatibility is " + project.sourceCompatibility

    def lintTasks = sourceSets.collect { sourceSet ->
      def srcDirs = sourceSet.java.srcDirs.findAll { dir -> dir.exists() }

      tasks.create(sourceSet.getTaskName("ecjLint", null), JavaExec, {task ->
....
...

Output

> Configure project :solr:server
The project name is project ':solr:server' and the sourceCompatibility is 21

> Configure project :solr:core
The project name is project ':solr:core' and the sourceCompatibility is 21

> Configure project :solr:api
The project name is project ':solr:api' and the sourceCompatibility is 17

> Configure project :solr:solrj
The project name is project ':solr:solrj' and the sourceCompatibility is 17

The version has changed for SolrJ and for api as well. However, as @dsmiley pointed out before that test classes for both of these project should point to 21. Still looking for a way How we can achieve that.

iamsanjay avatar Jun 10 '24 03:06 iamsanjay