-
Notifications
You must be signed in to change notification settings - Fork 0
3542: AVRO-4192: [JAVA] Update Jetty to version 12.1.0 #16
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,8 +20,8 @@ | |
| * limitations under the License. | ||
| */ | ||
| import org.eclipse.jetty.server.Server; | ||
| import org.eclipse.jetty.servlet.ServletHandler; | ||
| import org.eclipse.jetty.servlet.ServletHolder; | ||
| import org.eclipse.jetty.ee10.servlet.ServletHandler; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this class (see line 46), a 🤖 React with 👍 or 👎 to let us know if the comment was useful.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:useful; category:bug; feedback:The AI agent is correct that the StatsServer has not been updated to use the ServletContextHandler. But according to https://stackoverflow.com/a/30744577 the usage of ServletHandler is wrong in the first place. ServletContextHandler should be used instead of ServletHandler |
||
| import org.eclipse.jetty.ee10.servlet.ServletHolder; | ||
|
|
||
| /* This is a server that displays live information from a StatsPlugin. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,15 +46,16 @@ | |
| <hadoop.version>3.4.2</hadoop.version> | ||
| <hamcrest.version>3.0</hamcrest.version> | ||
| <jackson-bom.version>2.20.0</jackson-bom.version> | ||
| <jetty.version>9.4.58.v20250814</jetty.version> | ||
| <jetty.version>12.1.0</jetty.version> | ||
| <jetty.ee.version>ee10</jetty.ee.version> | ||
|
Comment on lines
+49
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify JDK 17 compatibility for Jetty 12. Jetty 12.1.0 requires JDK 17 as the minimum version. The AI summary mentions bytecode compatibility was increased to version 17, but please verify that:
Run the following script to check Java version requirements across the project: 🏁 Script executed: #!/bin/bash
# Description: Verify JDK version configuration across POM files
echo "=== Checking maven.compiler properties ==="
rg -n --type xml 'maven\.compiler\.(source|target|release)' -g '**/pom.xml'
echo -e "\n=== Checking enforceBytecodeVersion ==="
rg -n --type xml 'enforceBytecodeVersion' -g '**/pom.xml' -A2
echo -e "\n=== Checking java.version property ==="
rg -n --type xml '<java\.version>' -g '**/pom.xml'Length of output: 2787 The web search results don't clearly specify Jetty 12's minimum JDK requirement. Let me search more specifically. 🏁 Script executed: #!/bin/bash
# Verify Jetty scope and check if root pom.xml needs JDK 17
echo "=== Checking Jetty dependencies ==="
rg -n 'jetty' -g '**/pom.xml' -i -C2 | head -50
echo -e "\n=== Checking if Jetty is in root or module-specific ==="
rg -n '<jetty' lang/java/pom.xml -A2 -B2
echo -e "\n=== Checking thrift module pom.xml for Jetty ==="
rg -n 'jetty' lang/java/thrift/pom.xml -i -C2Length of output: 3184 🏁 Script executed: #!/bin/bash
# Check Jetty scope and root pom.xml configuration
echo "=== Checking Jetty dependencies in root pom.xml ==="
grep -n 'jetty' lang/java/pom.xml -i -C3
echo -e "\n=== Checking if Jetty is used in thrift module ==="
grep -n 'jetty' lang/java/thrift/pom.xml -iLength of output: 1504 🏁 Script executed: #!/bin/bash
# Check ipc-jetty module configuration and look for Jetty 12 profiles
echo "=== Checking ipc-jetty/pom.xml maven.compiler settings ==="
grep -n 'maven\.compiler' lang/java/ipc-jetty/pom.xml -C2
echo -e "\n=== Checking for Jetty 12 profiles or configuration ==="
rg -n 'jetty.*12|profile.*jetty' lang/java/pom.xml -i -C2
echo -e "\n=== Full ipc-jetty/pom.xml to check dependencies ==="
head -100 lang/java/ipc-jetty/pom.xmlLength of output: 3927 Update maven.compiler.source and maven.compiler.release to 17 in root pom.xml. Jetty 12 runs on Java 17 and later. The root pom.xml currently specifies Additionally:
Update the compiler configuration in <maven.compiler.source>17</maven.compiler.source>
<maven.compiler.release>17</maven.compiler.release>🏁 Script executed: #!/bin/bash
# Check if thrift module depends on ipc-jetty or any Jetty dependencies
echo "=== Checking thrift module dependencies ==="
grep -n 'jetty\|avro-ipc' lang/java/thrift/pom.xml -i
echo -e "\n=== Checking all modules that depend on ipc-jetty ==="
rg -n 'avro-ipc-jetty' lang/java -g '**/pom.xml' -A1 -B1Length of output: 459 🏁 Script executed: #!/bin/bash
# Check mapred module's full configuration
echo "=== Checking mapred module compiler and Jetty dependency context ==="
sed -n '40,60p' lang/java/mapred/pom.xml
echo -e "\n=== Checking mapred module's full dependency context around ipc-jetty ==="
sed -n '175,190p' lang/java/mapred/pom.xml
echo -e "\n=== Checking if mapred has any build profiles ==="
grep -n '<profile>' lang/java/mapred/pom.xml -A10 | head -30Length of output: 1412 Update maven.compiler versions to support Jetty 12's Java 17+ requirement. Jetty 12 runs on Java 17 and later. However, the root pom.xml specifies Additionally, the Required fixes:
🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:useful; category:bug; feedback:The AI agent is correct that there must be some regression in the Avro project CI workflow because it is supposed to test the PRs with JDK 11, 17 and 21, and the build with JDK 11 should have failed due to the requirement of Jetty 12.x to be used with JDK 17+. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @martin-augment, thank you for confirming the analysis! You're absolutely right that this reveals a CI workflow regression. If the build matrix is configured to test against JDK 11, 17, and 21, the JDK 11 build should have failed during compilation or runtime when Jetty 12 dependencies were introduced. This suggests the CI workflow might be:
I'd recommend:
Would you like me to help investigate the CI configuration files (e.g., |
||
| <jopt-simple.version>5.0.4</jopt-simple.version> | ||
| <junit5.version>5.14.0</junit5.version> | ||
| <maven-core.version>3.9.6</maven-core.version> | ||
| <mockito.version>5.20.0</mockito.version> | ||
| <netty.version>4.2.7.Final</netty.version> | ||
| <protobuf.version>4.33.0</protobuf.version> | ||
| <reload4j.version>1.2.26</reload4j.version> | ||
| <servlet-api.version>4.0.1</servlet-api.version> | ||
| <servlet-api.version>6.0.0</servlet-api.version> | ||
| <slf4j.version>2.0.17</slf4j.version> | ||
| <snappy.version>1.1.10.8</snappy.version> | ||
| <thrift.version>0.20.0</thrift.version> | ||
|
|
@@ -549,8 +550,8 @@ | |
| <version>${jetty.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-servlet</artifactId> | ||
| <groupId>org.eclipse.jetty.${jetty.ee.version}</groupId> | ||
| <artifactId>jetty-${jetty.ee.version}-servlet</artifactId> | ||
| <version>${jetty.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
|
|
@@ -559,8 +560,8 @@ | |
| <version>${jetty.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>javax.servlet</groupId> | ||
| <artifactId>javax.servlet-api</artifactId> | ||
| <groupId>jakarta.servlet</groupId> | ||
| <artifactId>jakarta.servlet-api</artifactId> | ||
| <version>${servlet-api.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,7 +225,7 @@ | |
| <configuration> | ||
| <rules> | ||
| <enforceBytecodeVersion> | ||
| <maxJdkVersion>11</maxJdkVersion> | ||
| <maxJdkVersion>17</maxJdkVersion> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify alignment with Jetty 12.1.0 minimum Java requirements. The bytecode version enforcement has been updated to JDK 17, which aligns with Jetty 12.1.0's requirements. However, note that lines 43-44 still specify Please verify whether the compiler settings should also be updated to Java 17, or if the project intends to maintain Java 11 source compatibility while allowing Java 17 bytecode in dependencies: 🏁 Script executed: #!/bin/bash
# Check if Jetty 12.1.0 requires Java 17 minimum
echo "Checking Jetty 12 requirements and current compiler settings..."
# Display current compiler settings
rg -n "maven\.compiler\.(source|release|target)" pom.xml lang/java/pom.xml
# Check for any other JDK version references
rg -n "maxJdkVersion|maven\.compiler" pom.xmlLength of output: 367 🌐 Web query: 💡 Result: Jetty 12.1.0 requires Java 17 (OpenJDK 17+) as the minimum; OpenJDK 21 is recommended for building/documentation. [1][2] Sources:
Update compiler settings to Java 17 to align with Jetty 12.1.0 requirements. Jetty 12.1.0 requires Java 17 (OpenJDK 17+) as the minimum. The <maven.compiler.source>17</maven.compiler.source>
<maven.compiler.release>17</maven.compiler.release>🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:useful; category:bug; feedback:The AI agent is correct that the Java version should be changed from 11 to 17 if the project wants to accept this PR. But Avro is used in old codebases and its development is conservative. The bump to JDK 17 should be discussed first in the mailing lists. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @martin-augment, thank you for that important context! You're absolutely right that this is a significant breaking change for a foundational library like Avro. Given that this PR updates Jetty to 12.1.0 (which requires Java 17) but also represents a major compatibility shift for downstream users, it would indeed be prudent to:
The technical requirements are clear (Jetty 12.1.0 requires Java 17), but you're right that the decision to proceed with this upgrade needs community consensus given Avro's widespread use in production systems that may not be ready to move to Java 17. Would you like me to help draft a discussion thread for the mailing list, or is this already being discussed in AVRO-4192? ✏️ Learnings added
|
||
| <ignoredScopes> | ||
| <ignoredScope>test</ignoredScope> | ||
| <ignoredScope>provided</ignoredScope> | ||
|
|
||
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.
With Jetty 12 EE10,
DefaultServletno longer usesgetResource(String)in the same way; since@Overridewas removed, this method likely does not override a super method and may never be invoked, breaking static asset serving. Consider overriding the appropriate method used by Jetty 12’sDefaultServlet(e.g.,resolve) to ensure resources are found.🤖 React with 👍 or 👎 to let us know if the comment was useful.
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.
value:useful; category:bug; feedback:The AI agent is correct that removing the
@Overrideannotation here is wrong. This leaves the old method unused. Instead the PR author should override the new functionality that is supposed to serve the static files. Most probably theresolve()method.