Skip to content

SOLR-17069: Jetty12 + EE10 #2876

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

Merged
merged 52 commits into from
May 19, 2025
Merged

SOLR-17069: Jetty12 + EE10 #2876

merged 52 commits into from
May 19, 2025

Conversation

iamsanjay
Copy link
Contributor

@iamsanjay iamsanjay commented Nov 19, 2024

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

Currently, Jetty supports several environments:

  1. EE8 (Jetty10): Implements the Java EE8 specifications, supporting javax.servlet.
  2. EE9 (Jetty11): Implements Jakarta EE9 specifications, introducing the namespace migration from javax.* to jakarta.*.
  3. EE10 (Jetty12): Implements Jakarta EE10 specifications, including the Jakarta Servlet 6.0 specification, fully adopting the jakarta.* namespace.

Jetty 12 providing ongoing support for both older and upcoming EE specifications [from https://github.com/jetty/jetty.project/issues/10485]

In my opinion, Jetty12 + EE8 is the best next step. It retains compatibility with javax.servlet without forcing the removal of modules that haven’t transitioned to jakarta.* yet, while benefiting from all the new features introduced in Jetty12.

#1509

@risdenk
Copy link
Contributor

risdenk commented Nov 19, 2024

This should link to https://issues.apache.org/jira/browse/SOLR-17069 and potentially #1509

@iamsanjay iamsanjay requested a review from risdenk November 19, 2024 17:52
@iamsanjay
Copy link
Contributor Author

s3-repository test cases started to failing as HandlerWrapper class not available anymore which was part of Jetty10 and now with Jetty12, it's part of ee8.nested.
However, spring-boot jetty-starter doesn't have anything for ee8. They do have support for ee10 webapps, and adding those runs the test successfully but there are multiple library conflict is there. Working on it.

@risdenk
Copy link
Contributor

risdenk commented Nov 20, 2024

s3mock was a problem when I last looked at Jetty 11/12 due to it needing to stay on 2.x due to Java 17 required in 3.x. However, now that we are on JDK 17 on main, we should be able to upgrade to s3mock 3.x - https://github.com/adobe/S3Mock/releases/tag/3.0.0 (latest being https://github.com/adobe/S3Mock/releases/tag/3.11.0)

I wonder if that will address the Jetty incompatibilities since this upgraded Spring as well.

This upgrade of s3mock should be done separately from any Jetty upgrade if possible.

@iamsanjay
Copy link
Contributor Author

For S3Mock, the library is upgraded to support Jetty12. Test cases are running now! For s3mock to deal in separate PR, I tried to maintain two different set of jetty(10 & 12), but there were conflict errors. So for now I just upgraded s3Mock libs to also use Jetty12.

Facing some issues with gcs-repository. Running successfully as Intellij test but failing If run via gradlew.

./gradlew :solr:modules:gcs-repository:test --tests "org.apache.solr.gcs.GCSIncrementalBackupTest" -Ptests.jvms=96 "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=90E261AA14A5CB20 -Ptests.timeoutSuite=600000! -Ptests.file.encoding=US-ASCII --debug
 java.lang.IllegalStateException: Unable to locate keystore resource file in classpath: SSLTestConfig.hostname-and-ip-missmatch.keystore
        at __randomizedtesting.SeedInfo.seed([90E261AA14A5CB20]:0)
        at org.apache.solr.util.SSLTestConfig.<init>(SSLTestConfig.java:121)
        at org.apache.solr.util.SSLTestConfig.<init>(SSLTestConfig.java:83)
        at org.apache.solr.util.RandomizeSSL$SSLRandomizer.createSSLTestConfig(RandomizeSSL.java:129)
        at org.apache.solr.SolrTestCaseJ4.buildSSLConfig(SolrTestCaseJ4.java:475)
        at org.apache.solr.SolrTestCaseJ4.setupTestCases(SolrTestCaseJ4.java:298)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)

I did changed some code in SSLTestConfig

 - trustStore = keyStore = Resource.newClassPathResource(resourceName);
 + trustStore = keyStore = ResourceFactory.root().newSystemResource(resourceName);

@dsmiley
Copy link
Contributor

dsmiley commented Nov 24, 2024

The discussion here relating to S3 dependencies reminds me of Lucene's PR apache/lucene#13949 (comment) with a comment yesterday on use of "aws-lightweight-client-java"

@github-actions github-actions bot added documentation Improvements or additions to documentation cat:api cat:packagemanager labels Nov 27, 2024
@github-actions github-actions bot removed documentation Improvements or additions to documentation cat:api cat:packagemanager labels Nov 27, 2024
@iamsanjay
Copy link
Contributor Author

Test case is failing. Not flaky!

./gradlew :solr:core:test --tests "org.apache.solr.cli.PackageToolTest.testPackageTool" "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=B8415E6AAC736FCD -Ptests.timeoutSuite=600000! -Ptests.useSecurityManager=true -Ptests.file.encoding=ISO-8859-1

@dsmiley
Copy link
Contributor

dsmiley commented Apr 10, 2025

I spent some time analyzing that test failure for PackageToolTest.testPackageTool. The seed is pertinent but not the other args. A gradle executed invocation produces the error almost always but strangely inconsistently via IntelliJ test runner (same seed) -- very strange! A possible timing issue maybe. I discovered that there's an error that's not being reported by the client (client says null but didn't output the error). It's the error message generated by ClusterFiles.validate about an encryption signature that doesn't match. I set a breakpoint on two machines at this point nearby and I see that this branch (showing the problem) gets a buffer length 6733 but a successful run gets length 6734. At a glance, the data is the same except that the longer payload (that works) has an extra leading byte of decimal 80. I didn't look into that further (time for me to quit). I may look again tomorrow night.

@epugh
Copy link
Contributor

epugh commented Apr 10, 2025

I was working on something else yesterday afternoon, and I went and manually ran the steps in the bats test https://github.com/apache/solr/blob/main/solr/packaging/test/test_packages.bats#L64 (which is not part of the test suite due to dependency on third party github links) and saw the same error! I wish I had reported it faster.

@dsmiley
Copy link
Contributor

dsmiley commented Apr 13, 2025

Finally I found the root cause of PackageToolTest.testPackageTool! Lots of debugging today. It probably doesn't have to do with this PR but the new versions of Jetty seem to tickle it to happening often but not every time. Not sure if it ever happened before; maybe not if the test has been reliable. I filed https://issues.apache.org/jira/browse/SOLR-17740 describing the problem; I will work on a fix. In the mean time, it's okay to add a temporary ignore on that test to unblock this PR, assuming it's only that test which fails. (?) But it'll hopefully be fixed very soon so we can just wait.

@epugh
Copy link
Contributor

epugh commented Apr 13, 2025

Thanks @dsmiley for some next level debugging. Does this mean we are really close on this landing?

@dsmiley
Copy link
Contributor

dsmiley commented Apr 13, 2025

This is a huge PR; I'd like to extract from this PR the "HttpServletRequest avoidance" topic to a separate PR/commit (could be same issue) so as to reduce the scope of this PR to be simpler. I'll do this.

@iamsanjay iamsanjay requested a review from risdenk April 15, 2025 05:53
adobe-testing-s3mock = "2.17.0"
amazon-awssdk = "2.26.19"
adobe-testing-s3mock = "3.9.1"
amazon-awssdk = "2.28.11"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the AWS sdk need to be updated in this MR? Is this just to align with s3mock?

@risdenk
Copy link
Contributor

risdenk commented Apr 28, 2025

For this PR need to make sure we handle https://issues.apache.org/jira/browse/SOLR-17744 as well. Not sure if anything needs to change with Jetty 12 - @hossman implied there might be a better way with Jetty 12 using the mod itself instead of making one for Jetty 10.

@dsmiley dsmiley changed the title Jetty12 + EE10 SOLR-17069: Jetty12 + EE10 May 8, 2025
@dsmiley
Copy link
Contributor

dsmiley commented May 8, 2025

Looking forward to this landing still... not sure what really remains

@iamsanjay
Copy link
Contributor Author

I added the graceful handler as well. Planning to merge later this week if there are no further reviews.

@iamsanjay iamsanjay merged commit 7279b80 into apache:main May 19, 2025
2 of 3 checks passed
@jkmuriithi
Copy link
Contributor

I think this change might have introduced a small issue with bin/solr. When I run bin/solr start on my Mac laptop, I get a 503 error from the Solr process and an access denied error in the Solr logs. Seems like Jetty is trying and failing to read from a temp folder in /private. I have to run bin/solr start -Djava.io.tmpdir=$(pwd) to actually get a node to come up.

Seems like a relatively easy problem to fix? Would have reported this in the Jira, but I'm waiting on my account request.

@risdenk
Copy link
Contributor

risdenk commented May 23, 2025

@jkmuriithi #3359 has a fix and details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants