DRILL-8540: Update Jetty to Version 12#3031
Conversation
cgivre
added
dependencies
backport-to-stable
joakime
left a comment
joakime
left a comment
There was a problem hiding this comment.
Overall a great changeset.
Just a few suggestions.
| <jersey.version>2.40</jersey.version> | ||
| <jetty.version>9.4.56.v20240826</jetty.version> | ||
| <jersey.version>3.1.9</jersey.version> | ||
| <jetty.version>12.0.15</jetty.version> |
There was a problem hiding this comment.
btw, Jetty has BOM dependencies that can be used as well.
One for core, and one for the environment you choose to use.
Would you be interested in that?
| verify(response, never()).sendError(401); | ||
| verify(response, never()).setHeader(HttpHeader.WWW_AUTHENTICATE.asString(), HttpHeader.NEGOTIATE.asString()); | ||
| // This test needs to be rewritten for Jetty 12 API | ||
| // TODO: Rewrite for Jetty 12 |
There was a problem hiding this comment.
It's often easier, and faster to execute, using a real server instance + using an HTTP client to connect to it.
| verify(response, never()).sendError(401); | ||
| verify(response, never()).setHeader(HttpHeader.WWW_AUTHENTICATE.asString(), HttpHeader.NEGOTIATE.asString()); | ||
| // This test needs to be rewritten for Jetty 12 API | ||
| // TODO: Rewrite for Jetty 12 |
There was a problem hiding this comment.
Directly mocking the HttpServletRequest isn't a great choice for testing.
It doesn't test the actual implementation, which depends heavily on internal HttpServletRequest instance types.
Using a real server, and real HTTP requests, is often faster to setup, and test.
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/SpnegoSecurityHandler.java
Outdated
Show resolved
Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/RolePrincipal.java
Outdated
Show resolved
Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/LogInLogOutResources.java
Outdated
Show resolved
Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/LogInLogOutResources.java
Outdated
Show resolved
Hide resolved
drill-yarn/src/main/java/org/apache/drill/yarn/appMaster/http/WebServer.java
Show resolved
Hide resolved
|
@joakime Thanks for the review. I believe I addressed all your comments. |
|
@joakime Thanks for your review. All unit tests are now passing again, and I believe it is ready for another look. |
|
@joakime I hope all is well. Do you think you'll have some time to do another review? Thanks! |
|
@pjfanning Would you be willing to do a final review of this? I addressed @joakime's comments but he said he doesn't have the time to review. |
3 participants
DRILL-8540: Update Jetty to Version 12
Description
This PR completes the Jetty 12 upgrade by updating drill-yarn's WebServer to use Jakarta EE 10 APIs and the new Jetty 12 servlet packages. Key changes include:
org.eclipse.jetty.ee10.servletimports,LoginService.login()method signatures, adopting the new ResourceFactory API, andServletContextHandlerconstructor usage.@Ignoreannotations due to an unresolvable conflict between Drill's Jetty 12 and Hadoop 3.x's Jetty 9 dependencies. These tests can be re-enabled when Hadoop upgrades to Jetty 12 (tracked in HADOOP-19625).Comprehensive documentation has been added in
docs/dev/Jetty12Migration.mdexplaining the changes, known limitations, and developer guidelines for working with Jetty 12.End of Java 11 Support
As Jetty 12 does not support Java < 17, this PR effectively terminates Drill's support for Java 11. Github actions have been adjusted accordingly. Note that Calcite is also migrating to Jetty 12, so if we wish to keep Drill in sync with Calcite, this is a necessary step.
Note on Splunk Tests
For some reason, the Jetty upgrade caused issues with the Splunk testing infrastructure. It is not clear why, but this has been resolved in this PR.
Documentation
No user facing changes.
Testing
Ran existing unit tests and tested WebUI manually.