Skip to content

3648: AVRO-4229 - Update StatsServer resource handler #34

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.

Sign up for GitHub

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

Open
martin-augment wants to merge 1 commit into
base: main
Choose a base branch
from
Open

3648: AVRO-4229 - Update StatsServer resource handler #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 0 additions & 45 deletions lang/java/ipc-jetty/src/main/java/org/apache/avro/ipc/jetty/StaticServlet.java
View file
Open in desktop

This file was deleted.

27 changes: 22 additions & 5 deletions lang/java/ipc-jetty/src/main/java/org/apache/avro/ipc/jetty/StatsServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,12 @@
* limitations under the License.
*/
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.servlet.ServletHandler;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.server.handler.HandlerList;
import org.eclipse.jetty.server.handler.ResourceHandler;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.servlet.ServletHolder;
import org.eclipse.jetty.util.resource.Resource;

/* This is a server that displays live information from a StatsPlugin.
*
Expand All @@ -42,11 +46,24 @@ public StatsServer(StatsPlugin plugin, int port) throws Exception {
this.httpServer = new Server(port);
this.plugin = plugin;

ServletHandler handler = new ServletHandler();
httpServer.setHandler(handler);
handler.addServletWithMapping(new ServletHolder(new StaticServlet()), "/");
ServletContextHandler servletContext = new ServletContextHandler(ServletContextHandler.SESSIONS);
servletContext.setContextPath("/");

handler.addServletWithMapping(new ServletHolder(new StatsServlet(plugin)), "/");
ServletHolder servletHolder = new ServletHolder(new StatsServlet(plugin));
servletContext.addServlet(servletHolder, "/");

ResourceHandler resourceHandler = new ResourceHandler();
Copy link
Copy Markdown

@augmentcode augmentcode bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

resourceHandler.setBaseResource(Resource.newClassPathResource("/org/apache/avro/ipc/stats/static"));
Copy link
Copy Markdown

@augmentcode augmentcode bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

resourceHandler.setDirectoriesListed(false); // Optional: prevent directory listing

ContextHandler staticContext = new ContextHandler();
staticContext.setContextPath("/static");
staticContext.setHandler(resourceHandler);

HandlerList handlers = new HandlerList();
handlers.setHandlers(new org.eclipse.jetty.server.Handler[] { staticContext, servletContext });
Comment on lines +63 to +64
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
HandlerList handlers = new HandlerList();
handlers.setHandlers(new org.eclipse.jetty.server.Handler[] { staticContext, servletContext });
HandlerList handlers = new HandlerList(staticContext, servletContext);

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


httpServer.setHandler(handlers);

httpServer.start();
}
Expand Down