-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Jetty12 (Jakarta EE8) upgrade #18424
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
base: master
Are you sure you want to change the base?
Conversation
// Create the core set of modules shared by all services. | ||
// Here and below, the modules are filtered by the load modules list and | ||
// the set of roles which this server provides. | ||
CoreInjectorBuilder coreBuilder = new CoreInjectorBuilder(childInjector, nodeRoles).forServerWithoutJetty(); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
CoreInjectorBuilder.forServerWithoutJetty
…llow backwards compatibility
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.
Pull Request Overview
This PR upgrades Apache Druid from Jetty 9 to Jetty 12 with Jakarta EE8 compatibility to minimize breaking changes while modernizing the web server infrastructure. Since Jetty 12 requires Java 17 runtime, Java 11 support is being dropped except for Hadoop ingestion tasks which can continue running on Java 11 YARN clusters.
Key changes:
- Updates Jetty dependencies from 9.4.x to 12.0.25 with Jakarta EE8 modules
- Replaces Avatica dependencies with custom JDBC handlers to avoid compatibility issues
- Adds configuration for URI compliance to maintain backward compatibility with legacy URI formats
Reviewed Changes
Copilot reviewed 73 out of 73 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
pom.xml | Updates Jetty version to 12.0.25, switches to EE8 modules, adds servlet-api exclusions |
licenses.yaml | Updates license information for new Jetty and Avatica versions |
server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java | Adds uriCompliance configuration property with LEGACY default |
server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java | Updates Jetty server configuration for v12 API changes |
sql/src/main/java/org/apache/druid/sql/avatica/*.java | Replaces Avatica handler inheritance with custom implementations |
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java | Updates for new Jetty client API |
indexing-hadoop/src/main/java/org/apache/druid/indexer/*.java | Removes Jetty dependencies and uses standard HTTP client for Hadoop compatibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
if (serverResponse.getHeaders().contains(headerName) && proxyResponse.containsHeader(headerName)) { | ||
// In Jetty 12 EE8, use the standard servlet API method to remove headers | ||
proxyResponse.setHeader(headerName, null); |
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.
Setting a header to null
may not reliably remove it across all servlet implementations. Consider using a more explicit removal method or documenting this behavior's dependency on the specific servlet container.
if (serverResponse.getHeaders().contains(headerName) && proxyResponse.containsHeader(headerName)) { | |
// In Jetty 12 EE8, use the standard servlet API method to remove headers | |
proxyResponse.setHeader(headerName, null); | |
proxyResponse.removeHeader(headerName); |
Copilot uses AI. Check for mistakes.
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.
I guess I need to dig deeper into this. I don't think we can do the cast to a jetty response object like we used to. And I don't want to reach into the ee8
internals either, even if something in there would provide an api for removing a header.
// Keep Druid jetty jars out of the classpath. They are not runnable in the < 17 hadoop java runtime | ||
if (jarFile.getName().endsWith(".jar") && !jarFile.getName().contains("jetty")) { |
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.
The string-based filtering !jarFile.getName().contains(\"jetty\")
is fragile and could accidentally exclude non-Jetty JARs with 'jetty' in their name. Consider using a more specific pattern match or maintaining an explicit list of JARs to exclude.
Copilot uses AI. Check for mistakes.
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.
I agree that this is suboptimal. I was in the "lets see if we can make it work" mindset. Maybe there is a more clean approach to filtering the jetty jars out.
} | ||
|
||
callback.succeeded(); | ||
return true; |
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.
The callback.succeeded() is called unconditionally, even for non-POST requests that don't write any response. This should only be called after successfully writing a response, or the logic should be restructured to handle different response scenarios correctly.
} | |
callback.succeeded(); | |
return true; | |
callback.succeeded(); | |
return true; | |
} else { | |
// For non-POST requests, return 405 Method Not Allowed | |
response.setStatus(405); | |
response.getHeaders().put("Content-Type", "application/json;charset=utf-8"); | |
String errorJson = "{\"error\": \"Method Not Allowed\"}"; | |
response.write(true, ByteBuffer.wrap(errorJson.getBytes(StandardCharsets.UTF_8)), callback); | |
callback.succeeded(); | |
return true; | |
} |
Copilot uses AI. Check for mistakes.
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.
latest commits should try to address this along with general lack of consistency in request handling between json and protobuf handlers.
Thanks for the PR, @capistrant ! I plan to finish a review today. |
Description
Upgrade to Jetty 12
Upgrades our jetty dependency to jetty 12.0.x (12.0.25 at time of writing). Uses Jakarta EE8 compatible jetty 12 to enable a less intrusive upgrade.
Avatica Changes
Calcite Avatica has a hard dependency on jetty9 still and there is no clear timeline on their upgrade plans. To avoid this being a blocker we have reduced our dependency footprint on Avatica in this PR by bringing the Jetty handler implementations for Druid JDBC while retaining compatibility with the Avatica JDBC client.
Hadoop Compatibility
Hadoop ecosystem also comes with lots of dependencies on jetty9 and java11 support. This section lays out implications for hadoop support
Deep storage
Confirmed functionality using Docker IT env
index_hadoop
Confirmed functionality using Docker IT env. We will want more expansive testing here. I tried to slim down the code that sets up the injector, but since the task needs to load all of the extensions, I ended up falling back to an ugly duplication of the service injector creation, just without jetty. I'm sure it can be improved if we want to.
Java 11 Support Implications
Jetty12 deps are in java 17 class file version, so running druid services on java11 seems out of the question. If this is the case, java 11 support will have to be dropped in 35.
Future Work
Release note
Upgrading Druid to Jetty 12 requires dropping Java 11 runtime support for Druid. The one exception to this is that Hadoop Ingestion can continue to run on Java 11 YARN clusters because the Jetty 12 dependency can be excluded from that runtime classpath safely.
A new server configuration option has been added with this PR:
druid.server.http.uriCompliance
. Jetty 12 by default has strict enforcement ofRFC3986
URI format. This is a change from Jetty 9. To retain compatibility with legacy Druid, we default this config toLEGACY
, which uses the more permissive URI format enforcement that jetty9 used. If the cluster you operate does not require LEGACY compatibility, it is recommended to use the upstream jetty default ofRFC3986
in your Druid deployment. Please see jetty documentation for more info.Upgrade Note
Java 11 runtime no longer supported. Java 17 is the only officially supported and recommended runtime environment for Druid 35
Key changed/added classes in this PR
PR is mostly a lot of tiny changes and tweaks. But some places to get close look could be:
This PR has: