-
Notifications
You must be signed in to change notification settings - Fork 743
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
Conversation
This should link to https://issues.apache.org/jira/browse/SOLR-17069 and potentially #1509 |
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java
Outdated
Show resolved
Hide resolved
solr/core/src/test/org/apache/solr/cloud/OverseerStatusTest.java
Outdated
Show resolved
Hide resolved
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java
Outdated
Show resolved
Hide resolved
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java
Outdated
Show resolved
Hide resolved
|
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. |
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java
Outdated
Show resolved
Hide resolved
solr/core/src/test/org/apache/solr/util/tracing/TestHttpServletRequestGetter.java
Outdated
Show resolved
Hide resolved
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.
I did changed some code in SSLTestConfig
|
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" |
e321ba0
to
31e58a1
Compare
Test case is failing. Not flaky!
|
I spent some time analyzing that test failure for |
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. |
Finally I found the root cause of |
Thanks @dsmiley for some next level debugging. Does this mean we are really close on this landing? |
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. |
adobe-testing-s3mock = "2.17.0" | ||
amazon-awssdk = "2.26.19" | ||
adobe-testing-s3mock = "3.9.1" | ||
amazon-awssdk = "2.28.11" |
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.
Does the AWS sdk need to be updated in this MR? Is this just to align with s3mock?
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. |
Looking forward to this landing still... not sure what really remains |
I added the graceful handler as well. Planning to merge later this week if there are no further reviews. |
I think this change might have introduced a small issue with Seems like a relatively easy problem to fix? Would have reported this in the Jira, but I'm waiting on my account request. |
@jkmuriithi #3359 has a fix and details |
https://issues.apache.org/jira/browse/SOLR-17069
Currently, Jetty supports several environments:
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