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.

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

Jetty12 + EE10 #2876

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Jetty12 + EE10 #2876

wants to merge 18 commits into from

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
gradle/libs.versions.toml Show resolved Hide resolved
@@ -305,7 +306,7 @@ commonsio-commonsio = { module = "commons-io:commons-io", version.ref = "commons
cybozulabs-langdetect = { module = "com.cybozu.labs:langdetect", version.ref = "cybozulabs-langdetect" }
dropwizard-metrics-core = { module = "io.dropwizard.metrics:metrics-core", version.ref = "dropwizard-metrics" }
dropwizard-metrics-graphite = { module = "io.dropwizard.metrics:metrics-graphite", version.ref = "dropwizard-metrics" }
dropwizard-metrics-jetty10 = { module = "io.dropwizard.metrics:metrics-jetty10", version.ref = "dropwizard-metrics" }
dropwizard-metrics-jetty10 = { module = "io.dropwizard.metrics:metrics-jetty12-ee10", version.ref = "dropwizard-metrics" }
Copy link
Contributor

Choose a reason for hiding this comment

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

does it matter that this is ee10 vs ee8 like the description?

Copy link
Contributor

Choose a reason for hiding this comment

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

also the key should change from dropwizard-metrics-jetty10

KNOWN_MIME_TYPES.add("text/xml");
KNOWN_MIME_TYPES.add("text/javascript");
KNOWN_MIME_TYPES.add("text/csv");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did csv/xhtml get added?

Copy link
Contributor

Choose a reason for hiding this comment

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

Starting from Jetty 11 these two mime types are not included in MimeTypes.Type, but we have tests that check if they are supported. Not sure if this could cause any issues with jetty though.

}

chain = injectJettyHandlers(chain);

if (config.enableV2) {
RewriteHandler rwh = new RewriteHandler();
rwh.setHandler(chain);
rwh.setRewriteRequestURI(true);
rwh.setRewritePathInfo(false);
// rwh.setRewriteRequestURI(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

commented out?

@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.

@@ -138,7 +139,7 @@ jayway-jsonpath = "2.9.0"
jctools = "4.0.5"
jersey = "3.1.9"
# TODO Sync with jersey versions
jersey-containers = "2.39.1"
jersey-containers = "3.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would go with jersey version 3.1.9 if possible, so that we can remove this entry completely (see line above).

gradle/libs.versions.toml Outdated Show resolved Hide resolved
KNOWN_MIME_TYPES.add("text/xml");
KNOWN_MIME_TYPES.add("text/javascript");
KNOWN_MIME_TYPES.add("text/csv");
Copy link
Contributor

Choose a reason for hiding this comment

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

Starting from Jetty 11 these two mime types are not included in MimeTypes.Type, but we have tests that check if they are supported. Not sure if this could cause any issues with jetty though.

@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
@epugh
Copy link
Contributor

epugh commented Dec 9, 2024

I picked changes from #2835 i.e. Hadoop-auth module being removed and this one and pushed a new branch to my fork.
Branch contains code for Jetty 12 + EE10 (Jakarta servlet api) https://github.com/iamsanjay/solr/tree/jetty12_ee10
I started working on ee8 because hadoop-auth module does not have any jakarta api support, however If #2835 will be merged then I believe we can transitioned to ee10 without any issue.

I've been sitting on merging the code, for no really good reason.. Thanks for highlighting that it's a bit of a blocker to your progress, I will update the PR and merge it today.

Merge is done!

@iamsanjay
Copy link
Contributor Author

c3c2f1c removed hadoop-auth
f7d900b is where the branch is upgraded to EE10, all the usage of javax.servlet has been replaced with jakarta.servlet

With this, changing the title of this PR as well.

@iamsanjay iamsanjay changed the title [WIP] Jetty12 + EE8 [WIP] Jetty12 + EE10 Dec 10, 2024
@iamsanjay
Copy link
Contributor Author

iamsanjay commented Jan 17, 2025

Most of the test cases, I believe, are failing due to some issue with the Restore Command. Now in the logs, there is not much details regarding the issue and the only thing I got is.

{responseHeader={status=0, QTime=611}, STATUS=failed, response=Error handling 'RESTORECORE' action}

But If I try to debug I can see more elaborated exception message which I believe for some reason was suppressed.

Cannot invoke "org.eclipse.jetty.server.Request.getAttribute(String)" because the return value of "org.eclipse.jetty.ee10.servlet.ServletApiRequest.getRequest()" is null

There is somewhat related issue opened on Jetty's github jetty/jetty.project#12518

@iamsanjay iamsanjay changed the base branch from main to branch_9_8 January 19, 2025 06:38
@iamsanjay iamsanjay changed the base branch from branch_9_8 to main January 19, 2025 06:38
@iamsanjay
Copy link
Contributor Author

Got some of the commits from the main branch. It would be great to remove those before someone would review this PR again.

@iamsanjay iamsanjay marked this pull request as ready for review January 19, 2025 07:35
@iamsanjay
Copy link
Contributor Author

The only failing test is the flaky one, I checked on develocity.

Reproduce with: ./gradlew :solr:modules:ltr:test --tests "org.apache.solr.ltr.TestLTRQParserPlugin.ltr_expensiveFeatureRescoringAndPartialResultsNotTolerated_shouldRaiseException" "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=690CA0CDF5423C59 -Ptests.timeoutSuite=600000! -Ptests.file.encoding=UTF-8

@iamsanjay iamsanjay changed the title [WIP] Jetty12 + EE10 Jetty12 + EE10 Jan 20, 2025
@iamsanjay iamsanjay requested a review from risdenk January 20, 2025 14:45
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.

7 participants