Upgrade to Jetty 12 #99

ZacBlanco · 2025-03-18T22:23:27Z

This PR moves airlift from Jetty 9.X to Jetty 12, the latest stable Jetty release. It provides a wide range of improvements

improved resource utilization and performance (https://webtide.com/jetty-12-performance-figures/)
security vulnerability fixes
move from the deprecated javax.* APIs to jakarta.*

It also however requires significant refactoring of not just the Jetty libraries, but also moving things like javax servlets to the jakarta namespace as well as bumping the minimum bytecode version on components which rely on jetty to Java 17.

auden-woolfson

For first half of PR. Will add more later when I get the chance

concurrent/src/main/java/com/facebook/airlift/concurrent/AsyncSemaphore.java

dbpool/src/main/java/com/facebook/airlift/dbpool/H2EmbeddedDataSource.java

event/src/main/java/com/facebook/airlift/event/client/JsonEventWriter.java

http-client/pom.xml

ScrapCodes

Hi @ZacBlanco , everything looks good !
Upgrading Jetty seems to be a cleaner way to upgrade. You have responded to the suggestions from modernizer plugin.

ZacBlanco · 2025-03-24T21:51:46Z

I have a PR open to test the CI on this for now until we have a new release of airbase: #100

ZacBlanco · 2025-03-25T18:21:02Z

This PR is ready to be reviewed. The only required remaining change is to use the new airbase version in the root pom. Everything else should be set.

I am happy to re-arrange/squash commits as reviewers see fit, but I would like to wait until review is finished to squash/split any commits

ZacBlanco · 2025-03-27T23:28:53Z

I have a PR open to test the CI on this for now until we have a new release of airbase: #100

airbase 105 is now released- the CI should run successfully on this PR now

ZacBlanco · 2025-03-28T16:29:07Z

This is a large PR, so I want to provide some guidance for places reviewers should look
- Many changes are simply renaming javax.* to jakarta.*. I think files with these changes can be largely ignored.
- POMs should be examined, but most are just swapping javax dependencies for jakarta ones except:
  - replacing Apache bval jakarta.validation-api implementation with the Hibernate Validator implementation. This is because bval releases which support Java 17+ require Java 11 or 17 minimum bytecode which I didn't want to require for all modules yet.
  - swapping cglib-nodep for bytebuddy inside the configuration due to cglib incompatibilities on java 17.
  - Some modules are moved to bytecode level 17: http-utils, http-server, htt-client, event, discovery, dbpool, jaxrs-testing, jaxrs, jmx-http-rpc, jmx-http, jmx, sample-server, skeleton-server
  - Replacement dependency for parts of the javax.* annotation API. Some annotations were removed moving to the jakarta namespace. We still use some annotations from jakarta.annotation-api, but we use a new dependency for some that were removed. For example, the @ThreadSafe now uses com.google.errorprone:error_prone_annotations.
- Places with material code changes (mostly jetty/http bits). These are bits with more than simple renames/refactors
  - configuration/src/main/java/com/facebook/airlift/configuration/testing/ConfigAssertions.java
  - event/src/main/java/com/facebook/airlift/event/client/JsonEventWriter.java
  - http-client/src/main/java/com/facebook/airlift/http/client/jetty/AbstractContentProvider.java
  - http-client/src/main/java/com/facebook/airlift/http/client/jetty/AuthorizationPreservingHttpClient.java
  - http-client/src/main/java/com/facebook/airlift/http/client/jetty/BodyGeneratorContentProvider.java
  - http-client/src/main/java/com/facebook/airlift/http/client/jetty/ChunkedBytesContentProvider.java
  - http-client/src/main/java/com/facebook/airlift/http/client/jetty/JettyHttpClient.java
  - http-client/src/main/java/com/facebook/airlift/http/client/jetty/JettyLogging.java
  - http-client/src/main/java/com/facebook/airlift/http/client/spnego/SpnegoAuthenticationProtocolHandler.java
  - http-client/src/test/java/com/facebook/airlift/http/client/AbstractHttpClientTest.java
  - http-client/src/test/java/com/facebook/airlift/http/client/jetty/TestChunkedBytesContentProvider.java
  - http-client/src/test/java/com/facebook/airlift/http/client/jetty/TestHttpClientLogger.java
  - http-server/src/main/java/com/facebook/airlift/http/server/ClassPathResourceHandler.java
  - http-server/src/main/java/com/facebook/airlift/http/server/DelimitedRequestLog.java
  - http-server/src/main/java/com/facebook/airlift/http/server/HashLoginServiceProvider.java
  - http-server/src/main/java/com/facebook/airlift/http/server/HttpRequestEvent.java
  - http-server/src/main/java/com/facebook/airlift/http/server/HttpServer.java
  - http-server/src/main/java/com/facebook/airlift/http/server/HttpServerChannelListener.java
  - http-server/src/test/java/com/facebook/airlift/http/server/TestDelimitedRequestLog.java
  - http-server/src/test/java/com/facebook/airlift/http/server/TestHttpServerCipher.java
  - jmx/src/main/java/com/facebook/airlift/jmx/JmxAgent8.java
  - jmx/src/test/java/com/facebook/airlift/jmx/TestJmxAgent.java

Hopefully this is helpful.

aaneja

Short first pass

http-client/src/main/java/com/facebook/airlift/http/client/jetty/JettyHttpClient.java

aaneja

Flush 1

http-client/src/main/java/com/facebook/airlift/http/client/jetty/JettyHttpClient.java

http-server/src/main/java/com/facebook/airlift/http/server/HttpServer.java

http-client/src/main/java/com/facebook/airlift/http/client/jetty/JettyHttpClient.java

http-server/src/main/java/com/facebook/airlift/http/server/HttpServerChannelListener.java

prestodb-ci · 2025-04-08T16:23:53Z

@ethanyzhang imported this issue into IBM GitHub Enterprise

...lient/src/main/java/com/facebook/airlift/http/client/jetty/BodyGeneratorContentProvider.java

http-server/src/main/java/com/facebook/airlift/http/server/ClassPathResourceHandler.java

rschlussel · 2025-04-22T20:34:31Z

general question: does this mean that airlift will no longer work with java 8? How will that fit with keeping some parts of presto at language level 8?

ZacBlanco · 2025-04-22T20:42:24Z

Some modules are kept at language level 8, some will move to 17. It is controlled by the property: project.build.targetJdk. In airbase 105 the default targetJdk is still at 8. It must be explicitly set to 17 in order for the compiler and maven enforcer to work on dependencies and modules which use java 17 bytecode

These are the modules in airlift whose minimum bytecode compatibility is being moved to 17 (due to dependencies on jetty 12 or on another module which depends on http-client/server).

dbpool
discovery
event
http-client
http-server
http-utils
jaxrs
jaxrs-testing
jmx-http-rpc
jmx-http
jmx
sample-server
skeleton-server

You can search for project.build.targetJdk to see which modules will be released with 17-level bytecode.

As for Presto, I have a draft PR up which is passing most unit tests, including most PoS ones where the PoS JDK level is still set to 8. See prestodb/presto#24869

This commit upgrades the codebase to use jetty 12 along with all the dependent modules. There are a large number of changes outside of jetty that are required to comply with the updated APIs as well as some of the newer modernizer rules that execute with the target bytecode of some modules set to 17 The main types of changes are outlined below: 1. Nearly all usage of packages from javax.annotation, javax.validation javax.inject, javax.ws.rs-api, and javax.servlet have been replaced with the newer jakarta.* APIs. This is a requirement due to the newer jetty version requiring these libraries in order properly function. Some annotations have been removed entirely from the jakarta.annotation package, so they have been replaced with equivalent annotations from com.google.errorprone:error_prone_annotations package. Guice was upgraded to 7.0 to support the jakarta.inject namespace 2. Minimum bytecode level on some modules being raised to 17. Due to Jetty 12 being compiled for Java 17 bytecode, it means modules which rely on jetty directly and transitively must be compiled with java 17 bytecode as well. http-client and http-server are the modules with direct dependencies on jetty, but many other such as event, discovery, and more have their bytecode level increased in order to accomodate the jetty upgrade. Bytecode levels are set in the project's pom.xml <properties> tag using <project.build.targetJdk> 3. Modernizer changes. Due to modules being upgraded to Java 17 bytecode some new modernizer rules are now enforced to take advantage of the new Java 17 APIs. Many modules have slight changes to comply with our modernizer plugin. Most changes center around using Path instead of File and Java Streams API equivalents in place of Guava Iterables and Functions 4. The configuration's validator implementation is updated to use the hibernate validator instead of Apache bval-jsr. This is because the bval-jsr dependency does not support the jakarta namespace annotations at a java 8 bytecode level. Hibernate is used because it satisfies the requirement to keep this module at Java 8 bytecode. Aside from these, the rest of the changes center around supporting Jetty 12's new APIs within our http-client and http-server implementation. For example, supporting the new ContentProvider APIs and changing usages of deprecated or removed methods from Jetty 9.

The default previously was quiet period of 2 seconds and a timeout of 15 seconds. This cuts drift-integration-tests runtime from 7 minutes to about 10-20 seconds

aaneja · 2025-04-24T03:36:45Z

.../src/main/java/com/facebook/drift/transport/netty/client/DriftNettyMethodInvokerFactory.java

            connectionPoolMaintenanceExecutor.shutdownNow();
            try {
-                group.shutdownGracefully().await();
+                group.shutdownGracefully(0, 1, SECONDS).await();


Instead of changing the default, should we instead make it configurable so tests can shutdown quickly ?

Making it configurable is a good idea. We can probably just add some parameters like quietPeriodSeconds and shutdownTimeoutSeconds

auden-woolfson suggested changes Mar 18, 2025

View reviewed changes

auden-woolfson reviewed Mar 19, 2025

View reviewed changes

http-client/pom.xml Outdated Show resolved Hide resolved

ZacBlanco force-pushed the jetty-12-upgrade branch from 52acde5 to a53e209 Compare March 20, 2025 15:21

ScrapCodes approved these changes Mar 21, 2025

View reviewed changes

ZacBlanco force-pushed the jetty-12-upgrade branch from a53e209 to fa7934d Compare March 24, 2025 21:19

ZacBlanco force-pushed the jetty-12-upgrade branch from fa7934d to 02bb691 Compare March 25, 2025 18:17

ZacBlanco marked this pull request as ready for review March 25, 2025 18:21

ZacBlanco requested a review from a team as a code owner March 25, 2025 18:21

ZacBlanco requested a review from presto-oss March 25, 2025 18:21

ZacBlanco mentioned this pull request Mar 27, 2025

Upgrade dependencies in preparation for jetty 12 upgrade prestodb/airbase#30

Merged

ZacBlanco force-pushed the jetty-12-upgrade branch from ae7e9e7 to 844de99 Compare March 27, 2025 23:23

aaneja self-requested a review April 1, 2025 18:15

aaneja reviewed Apr 1, 2025

View reviewed changes

http-client/src/main/java/com/facebook/airlift/http/client/jetty/JettyHttpClient.java Show resolved Hide resolved

http-client/src/main/java/com/facebook/airlift/http/client/jetty/JettyHttpClient.java Show resolved Hide resolved

aaneja reviewed Apr 2, 2025

View reviewed changes

ZacBlanco requested a review from aaneja April 9, 2025 14:46

aaneja approved these changes Apr 22, 2025

View reviewed changes

...lient/src/main/java/com/facebook/airlift/http/client/jetty/BodyGeneratorContentProvider.java Outdated Show resolved Hide resolved

http-server/src/main/java/com/facebook/airlift/http/server/ClassPathResourceHandler.java Outdated Show resolved Hide resolved

ZacBlanco force-pushed the jetty-12-upgrade branch 3 times, most recently from e19d744 to 9545845 Compare April 22, 2025 18:07

aaneja mentioned this pull request Apr 22, 2025

Upgrade to Jetty 12 #103

Merged

ZacBlanco force-pushed the jetty-12-upgrade branch 3 times, most recently from 68e171c to 5e1a475 Compare April 23, 2025 06:31

ZacBlanco force-pushed the jetty-12-upgrade branch from 5e1a475 to 34dff03 Compare April 23, 2025 17:32

ZacBlanco added 4 commits April 23, 2025 13:27

Upgrade to airbase 105

07511f5

update github actions to java 17 only

4c601cf

Add http server config for uri compliance mode

7650fcc

ZacBlanco force-pushed the jetty-12-upgrade branch from 34dff03 to 7650fcc Compare April 23, 2025 23:10

Speed up shutdown time of drift server

c75bed4

The default previously was quiet period of 2 seconds and a timeout of 15 seconds. This cuts drift-integration-tests runtime from 7 minutes to about 10-20 seconds

aaneja reviewed Apr 24, 2025

View reviewed changes

ZacBlanco closed this Apr 24, 2025

Upgrade to Jetty 12 #99

Upgrade to Jetty 12 #99

Uh oh!

Conversation

ZacBlanco commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auden-woolfson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ScrapCodes left a comment

Choose a reason for hiding this comment

Uh oh!

ZacBlanco commented Mar 24, 2025

Uh oh!

ZacBlanco commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZacBlanco commented Mar 27, 2025

Uh oh!

ZacBlanco commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaneja left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aaneja left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prestodb-ci commented Apr 8, 2025

Uh oh!

Uh oh!

Uh oh!

rschlussel commented Apr 22, 2025

Uh oh!

ZacBlanco commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaneja Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

ZacBlanco Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ZacBlanco commented Mar 18, 2025 •

edited

Loading

auden-woolfson left a comment •

edited

Loading

ZacBlanco commented Mar 25, 2025 •

edited

Loading

ZacBlanco commented Mar 28, 2025 •

edited

Loading

ZacBlanco commented Apr 22, 2025 •

edited

Loading