3648: AVRO-4229 - Update StatsServer resource handler#34
3648: AVRO-4229 - Update StatsServer resource handler#34martin-augment wants to merge 1 commit intomainfrom
Conversation
Summary of ChangesHello @martin-augment, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Code Review - PR #34: AVRO-4229 - Update StatsServer resource handlerSummaryThis PR modernizes the static resource handling in ✅ Positive Aspects
🐛 Critical Issues1. Incorrect resource path (StatsServer.java:56)resourceHandler.setBaseResource(Resource.newClassPathResource("/org/apache/avro/ipc/stats/static"));Problem: The static resources are located in the Evidence: The static files are in Fix: You need to ensure the
2. Changed URL structure breaks existing clients (StatsServer.java:60)staticContext.setContextPath("/static");Problem: Previously, static files were served from the root path ( Impact: The Velocity templates in
These references will now break unless they're updated to use the Recommendation: Either:
3. License header placement (StatsServer.java:1-21)Problem: The license header is placed AFTER the import statements instead of at the beginning of the file. Current: package org.apache.avro.ipc.jetty;
import org.apache.avro.ipc.stats.StatsPlugin;
import org.apache.avro.ipc.stats.StatsServlet;
/*
* Licensed to the Apache Software Foundation (ASF)...Should be: /*
* Licensed to the Apache Software Foundation (ASF)...
*/
package org.apache.avro.ipc.jetty;
import org.apache.avro.ipc.stats.StatsPlugin;
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the StatsServer to use a more modern Jetty handler-based approach for serving the StatsServlet and static resources. The custom StaticServlet is removed and replaced with Jetty's standard ResourceHandler, which improves code clarity, maintainability, and security. The use of HandlerList to combine different handlers is a good practice. I have one minor suggestion to make the code even more concise.
| HandlerList handlers = new HandlerList(); | ||
| handlers.setHandlers(new org.eclipse.jetty.server.Handler[] { staticContext, servletContext }); |
There was a problem hiding this comment.
You can initialize the HandlerList and set the handlers in a single step using its constructor, which accepts a varargs Handler... argument. This makes the code more concise.
| HandlerList handlers = new HandlerList(); | |
| handlers.setHandlers(new org.eclipse.jetty.server.Handler[] { staticContext, servletContext }); | |
| HandlerList handlers = new HandlerList(staticContext, servletContext); |
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! The code could be simplified by using the overloaded constructor that accepts the list of contexts.
WalkthroughStaticServlet.java has been removed from the codebase. The StatsServer.java configuration was refactored to replace the previous ServletHandler approach with a HandlerList-based setup. The new configuration uses ServletContextHandler at the root path to host StatsServlet, and introduces a ResourceHandler backed by classpath resources to serve static files at the "/static" path. A ContextHandler wraps the static resource handler, and both contexts are combined using HandlerList as the HTTP server's handler. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Augment PR SummarySummary: Updates the Jetty-based stats UI server to use Jetty handlers instead of a custom static-file servlet. Changes:
Technical Notes: Static asset serving is now based on a classpath resource base ( 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| ServletHolder servletHolder = new ServletHolder(new StatsServlet(plugin)); | ||
| servletContext.addServlet(servletHolder, "/"); | ||
|
|
||
| ResourceHandler resourceHandler = new ResourceHandler(); |
There was a problem hiding this comment.
This effectively replaces (and deletes) the public StaticServlet; if org.apache.avro.ipc.jetty is considered a supported API surface, this is a binary-compatibility break worth double-checking.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:incorrect-but-reasonable; category:bug; feedback: The Augment AI reviewer is not correct! The "ipc-jetty" Maven module is an application, not a library, so there is no need to maintain API stability for it.
| servletContext.addServlet(servletHolder, "/"); | ||
|
|
||
| ResourceHandler resourceHandler = new ResourceHandler(); | ||
| resourceHandler.setBaseResource(Resource.newClassPathResource("/org/apache/avro/ipc/stats/static")); |
There was a problem hiding this comment.
Resource.newClassPathResource(...) can return null if the classpath base isn’t resolvable as a directory resource; if that happens the static UI assets may fail to serve (or cause runtime errors) without a clear message.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Augment AI reviewer is correct! Since the Java package is provided by another Maven module (the "ipc" module) it would be good to add a check that the result of Resource.newClassPathResource() is non-null and fail early at application start time if it is null for some reason. Prevents runtime exceptions at request time if the Java package is not found
value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! Since the Java package is provided by another Maven module (the "ipc" module) it would be good to add a check that the result of |
value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! The ASFv2 license should be placed at the top of the source files, not in the middle or at the end of their contents. Apparently the licence checker (the RAT plugin) does not detect this, so it should be fixed too. |
2 participants
3648: To review by AI