Jetty12 (Jakarta EE8) upgrade by capistrant · Pull Request #18424 · apache/druid

capistrant · 2025-08-21T03:19:45Z

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

Explore jetty 12 features and integrate them where appropriate

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 of RFC3986 URI format. This is a change from Jetty 9. To retain compatibility with legacy Druid, we default this config to LEGACY, 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 of RFC3986 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:

JDBC related files
- sql/src/main/java/org/apache/druid/sql/avatica/DruidAvaticaJsonHandler.java
- sql/src/main/java/org/apache/druid/sql/avatica/DruidAvaticaProtobufHandler.java
Hadoop Indexing related files
- indexing-hadoop/src/main/java/org/apache/druid/indexer/HadoopDruidIndexerConfig.java
- indexing-hadoop/src/main/java/org/apache/druid/indexer/JobHelper.java
- indexing-hadoop/src/main/java/org/apache/druid/indexer/Utils.java
- server/src/main/java/org/apache/druid/initialization/CoreInjectorBuilder.java
- server/src/main/java/org/apache/druid/initialization/Initialization.java
- server/src/main/java/org/apache/druid/initialization/ServerInjectorBuilder.java
server/src/main/java/org/apache/druid/guice/http/JettyHttpClientModule.java
server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerInitUtils.java
server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java

This PR has:

…xed up

…ully working

…sed on review

clintropolis · 2025-09-11T19:00:00Z

-  public static Handler getJettyRequestLogHandler()
-  {
-    // Ref: http://www.eclipse.org/jetty/documentation/9.2.6.v20141205/configuring-jetty-request-logs.html
-    RequestLogHandler requestLogHandler = new RequestLogHandler();
-    requestLogHandler.setRequestLog(new JettyRequestLog());
-
-    return requestLogHandler;
-  }


it looks like this is replaced with Server.setRequestLog in jetty 12, https://jetty.org/docs/jetty/12.1/programming-guide/migration/11-to-12.html#api-changes-handler-requestlog, do we need to add that somewhere like the server initializers class or something?

💯 definitely a miss by not properly migrating this. I think initialization in JettyServerModule is the right spot? Confirmed that it was wired up across all druid services in my Docker env after moving it in latest commits.

clintropolis · 2025-09-11T19:00:38Z

+then
+  DOCKER_COMPOSE='docker-compose'
+else
+  DOCKER_COMPOSE='docker compose'


shouldn't we prefer docker compose? i think docker-compose is pretty old now and hasn't been included in docker distribution for a while now

heh, ya. I probably just need to fix my local setup and not use docker-compose. If nothing else I'll flip it to only fall back to docker-compose.

clintropolis · 2025-09-11T19:00:43Z

-On Java 11, if these parameters are not included, you will see warnings like the following:
-
-```
-WARNING: An illegal reflective access operation has occurred
-WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
-WARNING: All illegal access operations will be denied in a future release
-```
-


I think you still see these if you don't add the stuff, are we just assuming that people are used to it by now?

ehh that was probably me just seeing Java 11 and not Java 11+ and hitting delete. I'll confirm it is still there and add back without the explicit java 11 part

actually maybe removing them is right if java 17 is actually throwing exceptions without them like we indicate on line 66

…r steps" This reverts commit bafb2fc.

capistrant · 2025-09-15T14:33:52Z

@kfaraz Thank you for the initial review! I made some modifications to the initialization code and avatica handler per your review. Let me know if this is going down the lines of what you were thinking.

kfaraz

Thanks for addressing the feedback, @capistrant !
I have left minor suggestions/queries in some places.
But none of those should block this PR.

kfaraz · 2025-09-11T04:36:02Z

 # See the License for the specific language governing permissions and
 # limitations under the License.

+# Determine if docker compose is available. If not, assume Docker supports


Does this change need to be included in this PR? Or can it be done separately too?

I ended up removing it and will not PR it separately. My machine just had a bad setup for docker, I assume most people use docker compose > docker-compose these days so supporting legacy is not needed. Especially since we are phasing out legacy ITs

kfaraz · 2025-09-18T07:56:50Z

+                <groupId>org.eclipse.jetty.ee8</groupId>
+                <artifactId>jetty-ee8-servlet</artifactId>
                <version>${jetty.version}</version>
+              <exclusions>


Nit: indentation seems off here.

kfaraz · 2025-09-18T07:57:25Z

+      // 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")) {


Could we use jetty-ee8-* instead?

kfaraz · 2025-09-18T07:59:07Z

+  /**
+   * Needed for Hadoop indexing that needs server-like Injector but can't run jetty 12
+   */
+  @VisibleForTesting


I think this method is used by HadoopDruidIndexerConfig too. If that is the case, let's remove this annotation.

kfaraz · 2025-09-18T08:00:24Z

+    return this.build(false);
+  }
+
+  public static Module registerNodeRoleModule(Set<NodeRole> nodeRoles)


It seems that this method is unchanged. Can we move it back to its original position so that we can avoid it from showing up in the diff?

kfaraz · 2025-09-18T08:20:58Z

+import org.apache.calcite.avatica.util.UnsynchronizedBuffer;
+import org.eclipse.jetty.server.Handler;
+
+public abstract class DruidAvaticaHandler extends Handler.Abstract implements MetricsAwareAvaticaHandler


Please add a short javadoc, if possible.

kfaraz · 2025-09-18T08:25:08Z

  public static final String AVATICA_PATH_NO_TRAILING_SLASH = "/druid/v2/sql/avatica-protobuf";
  public static final String AVATICA_PATH = AVATICA_PATH_NO_TRAILING_SLASH + "/";

+  private final ProtobufHandler pbHandler;


Suggested change

private final ProtobufHandler pbHandler;

private final ProtobufHandler protobufHandler;

kfaraz · 2025-09-18T08:26:40Z

+      try (Timer.Context ctx = this.requestTimer.start()) {
+        if (!"POST".equals(request.getMethod())) {
+          response.setStatus(405);
+          response.write(
+              true,
+              ByteBuffer.wrap("This server expects only POST calls.".getBytes(StandardCharsets.UTF_8)), callback
+          );
+          return true;
+        }
+        final byte[] requestBytes;
+        // Avoid a new buffer creation for every HTTP request
+        final UnsynchronizedBuffer buffer = threadLocalBuffer.get();
+        try (InputStream inputStream = Content.Source.asInputStream(request)) {
+          requestBytes = AvaticaUtils.readFullyToBytes(inputStream, buffer);
+        }
+        finally {
+          buffer.reset();
+        }
+
+        response.getHeaders().put("Content-Type", "application/octet-stream;charset=utf-8");
+
+        org.apache.calcite.avatica.remote.Handler.HandlerResponse<byte[]> handlerResponse;
+        try {
+          handlerResponse = pbHandler.apply(requestBytes);
+        }
+        catch (Exception e) {
+          LOG.debug(e, "Error invoking request");
+          handlerResponse = pbHandler.convertToErrorResponse(e);
+        }
+
+        response.setStatus(handlerResponse.getStatusCode());
+        response.write(true, ByteBuffer.wrap(handlerResponse.getResponse()), callback);
+        return true;


I wonder if parts of this logic can also live in the super class DruidAvaticaHandler. Not a blocker for this PR though.

kfaraz · 2025-09-18T08:27:13Z


  // Default implementation is for JSON to allow debugging of tests.
-  protected AbstractAvaticaHandler getAvaticaHandler(final DruidMeta druidMeta)
+  protected Handler.Abstract getAvaticaHandler(final DruidMeta druidMeta)


Suggested change

protected Handler.Abstract getAvaticaHandler(final DruidMeta druidMeta)

protected DruidAvaticaHandler getAvaticaHandler(final DruidMeta druidMeta)

kfaraz · 2025-09-18T08:28:37Z


  @Override
-  protected AbstractAvaticaHandler getAvaticaHandler(final DruidMeta druidMeta)
+  protected Handler.Abstract getAvaticaHandler(final DruidMeta druidMeta)


Suggested change

protected Handler.Abstract getAvaticaHandler(final DruidMeta druidMeta)

protected DruidAvaticaHandler getAvaticaHandler(final DruidMeta druidMeta)

* jetty 10 upgrade * checkpoint * compiling but broken cuz of at least one dep calcite depending on jetty9 * Fix breakage in router * Compilation fixes after merging master in. plus getting checkstyle fixed up * Remove some of the avatica deps blocking jetty12 * bump jetty to latest and fixup licenses.yaml * fixup after merging master * Fixup some licenses reporting issues * remove jetty dependency from indexing hadoop in attempt to keep it alive * stop doing static checks and build with jdk11 * Modifications to index_hadoop to try and keep it alive * Modify duplicate header removal strategy * Remove dependency on non public jetty nested packages * Revert back to verbose and dirty Injector setup to get hadoop index fully working * Fix http trailers for QueryResultPusher * Fix trailer related tests in QueryResourceTest * add some UTs * prevent banned dep from getting pulled in * Run rewrite rules mvn goal to fixup test files * stop accounting for buggy mock impl in sql test * Add new http server config for Jetty UriCompliance mode. Default to allow backwards compatibility * fix import order * fix spell checker flag * Fixup java docs links to use java17 * Unify approach to jetty handler for avatica json and protobuf * use 405 response code for avatica jdbc non post requests * fix checkstyle * Add link for new jetty related config * fix grammar in java support doc * Make the legacy ITs easier to run in a wider variety of docker steps * Cleanup some of the Injector initialization code for hadoop tasks, based on review * Starting point for creating a DruidAvaticaHandler base class * Share a little bit of the code between the avatica handlers * Work to fix AvaticaHandler constructor issues * Fix codeql flag * Revert "Make the legacy ITs easier to run in a wider variety of docker steps" This reverts commit bafb2fc. * Add RequestLogger configuration back using proper jetty12 mechanism * Fix checkstyle * fix import order * Address review comments * fix serialization (cherry picked from commit 7a084e6)

capistrant added 10 commits May 21, 2025 12:01

jetty 10 upgrade

ffdf8ee

checkpoint

f4eef97

compiling but broken cuz of at least one dep calcite depending on jetty9

63f23c9

Fix breakage in router

87e216d

Merge branch 'master' into jetty-update-august

c675117

Compilation fixes after merging master in. plus getting checkstyle fi…

acaf869

…xed up

Remove some of the avatica deps blocking jetty12

a741929

bump jetty to latest and fixup licenses.yaml

2536394

Merge branch 'master' into jetty12-ee8-upgrade

067b84b

fixup after merging master

a7a0134

capistrant added the WIP label Aug 21, 2025

github-actions Bot added Area - Batch Ingestion Area - Querying Area - Dependencies Area - Ingestion labels Aug 21, 2025

capistrant added 3 commits August 20, 2025 23:06

Fixup some licenses reporting issues

aba3dd0

remove jetty dependency from indexing hadoop in attempt to keep it alive

8042ece

stop doing static checks and build with jdk11

11d2964

github-actions Bot added the GHA label Aug 22, 2025

capistrant added 9 commits August 22, 2025 10:33

Modifications to index_hadoop to try and keep it alive

5f9ee32

Modify duplicate header removal strategy

1f13540

Remove dependency on non public jetty nested packages

74f5c09

Revert back to verbose and dirty Injector setup to get hadoop index f…

97e7649

…ully working

Fix http trailers for QueryResultPusher

0e08e10

Fix trailer related tests in QueryResourceTest

d2a5903

add some UTs

8ddb910

prevent banned dep from getting pulled in

a2a224d

Run rewrite rules mvn goal to fixup test files

8d9eb66

github-advanced-security AI found potential problems Aug 23, 2025

View reviewed changes

Comment thread server/src/main/java/org/apache/druid/initialization/ServerInjectorBuilder.java Fixed

stop accounting for buggy mock impl in sql test

3660520

capistrant added 7 commits September 9, 2025 11:40

Add link for new jetty related config

de76acc

fix grammar in java support doc

dec75f0

Make the legacy ITs easier to run in a wider variety of docker steps

bafb2fc

Cleanup some of the Injector initialization code for hadoop tasks, ba…

4ba80aa

…sed on review

Starting point for creating a DruidAvaticaHandler base class

36949b1

Share a little bit of the code between the avatica handlers

f154b04

Work to fix AvaticaHandler constructor issues

4506aac

github-advanced-security AI found potential problems Sep 10, 2025

View reviewed changes

Comment thread sql/src/main/java/org/apache/druid/sql/avatica/DruidAvaticaHandler.java Fixed

clintropolis reviewed Sep 11, 2025

View reviewed changes

capistrant added 4 commits September 11, 2025 15:28

Fix codeql flag

d2ac8b7

Revert "Make the legacy ITs easier to run in a wider variety of docke…

2ada34f

…r steps" This reverts commit bafb2fc.

Add RequestLogger configuration back using proper jetty12 mechanism

24f4c13

Fix checkstyle

a3bf74a

capistrant requested a review from kfaraz September 15, 2025 14:32

capistrant added 2 commits September 17, 2025 08:36

Merge branch 'master' into jetty12-ee8-upgrade

7b352db

fix import order

0b88098

kfaraz approved these changes Sep 18, 2025

View reviewed changes

capistrant added 3 commits September 18, 2025 11:09

Address review comments

a907ba7

fix serialization

73eab18

Merge branch 'master' into jetty12-ee8-upgrade

0f67db8

clintropolis approved these changes Sep 19, 2025

View reviewed changes

capistrant merged commit 7a084e6 into apache:master Sep 20, 2025
255 of 260 checks passed

capistrant mentioned this pull request Sep 20, 2025

Remove java 11 from all-jdks test matrix #18555

Merged

capistrant mentioned this pull request Oct 10, 2025

Allow configuration to disable SNI Host checking (defaulted to enabled) #18623

Merged

9 tasks

cecemei added this to the 35.0.0 milestone Oct 21, 2025

ashibhardwaj mentioned this pull request Dec 4, 2025

[Backport] Upgrade Jetty 9 to 12 #18809

Closed

This was referenced Dec 4, 2025

[Backport] Upgrade Jetty 9 to 12 confluentinc/druid#405

Closed

OBSDATA-12402 Upgrade Jetty 9 to 12 confluentinc/druid#408

Merged

		// 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")) {

	private final ProtobufHandler pbHandler;
	private final ProtobufHandler protobufHandler;

	protected Handler.Abstract getAvaticaHandler(final DruidMeta druidMeta)
	protected DruidAvaticaHandler getAvaticaHandler(final DruidMeta druidMeta)

Conversation

capistrant commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Upgrade to Jetty 12

Avatica Changes

Hadoop Compatibility

Deep storage

index_hadoop

Java 11 Support Implications

Future Work

Release note

Upgrade Note

Key changed/added classes in this PR

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capistrant commented Sep 15, 2025

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

capistrant commented Aug 21, 2025 •

edited

Loading