Skip to content

Conversation

aaneja
Copy link

@aaneja aaneja commented Apr 22, 2025

Clone PR of #99

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

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.

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.
@mosabua
Copy link

mosabua commented Apr 24, 2025

Please retain attribution in your commit to the original author or at least add @wendigo with the Co-authored-by: tag.

@ZacBlanco
Copy link

@mosabua I've written all this code myself. Can you point to which changes you think need to be attributed?

@wendigo
Copy link

wendigo commented Apr 24, 2025

@ZacBlanco configuration changes are exactly the same as in the airlift/airlift for example

@ZacBlanco
Copy link

That is not the case. One example at:

try (InputStream inputStream = newInputStream(Paths.get(path))) {

is different than in airlift/airlift at the same location.

https://github.com/airlift/airlift/blob/28168d00c0ff551675974f70b3b363317660f69e/configuration/src/main/java/io/airlift/configuration/ConfigurationLoader.java#L60-L62

I understand many changes will be similar due to mechanical renames from the javax to jakarta namespace, but there are plenty of places where I made changes that are very likely to be different from the upstream airlift repository. At no time while I authored this PR did I copy or cherry-pick any changes from airlift/airlift

@wendigo
Copy link

wendigo commented Apr 24, 2025


vs https://github.com/airlift/airlift/blob/28168d00c0ff551675974f70b3b363317660f69e/configuration-testing/src/main/java/io/airlift/configuration/testing/ConfigAssertions.java#L280

Even the exception message and extracted methods are the same (same structure, same naming, same variables) is so it's reasonable for me to have doubts.

@ZacBlanco ZacBlanco marked this pull request as ready for review April 24, 2025 13:54
@ZacBlanco ZacBlanco requested a review from a team as a code owner April 24, 2025 13:54
@ZacBlanco ZacBlanco self-requested a review April 24, 2025 13:54
@ZacBlanco ZacBlanco merged commit 02a7ffc into prestodb:master Apr 24, 2025
2 checks passed
@prestodb prestodb locked and limited conversation to collaborators Apr 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants