martin-g commented on code in PR #3648:
URL: https://github.com/apache/avro/pull/3648#discussion_r2802572823
##########
lang/java/ipc-jetty/src/main/java/org/apache/avro/ipc/jetty/StatsServer.java:
##########
@@ -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();
+
resourceHandler.setBaseResource(Resource.newClassPathResource("/org/apache/avro/ipc/stats/static"));
Review Comment:
```suggestion
Resource baseResource =
Resource.newClassPathResource("/org/apache/avro/ipc/stats/static");
if (baseResource == null) {
throw new IllegalStateException("Static resources not found on
classpath");
}
resourceHandler.setBaseResource(baseResource);
```
since the classpath is provided by another Maven module IMO it would be good
to make this check here and fail early if for some reason this package is not
available
##########
lang/java/ipc-jetty/src/main/java/org/apache/avro/ipc/jetty/StatsServer.java:
##########
Review Comment:
Not caused by this PR but we could improve it.
The ASFv2 licence should be at the top of the file.
##########
lang/java/ipc-jetty/src/main/java/org/apache/avro/ipc/jetty/StatsServer.java:
##########
@@ -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();
+
resourceHandler.setBaseResource(Resource.newClassPathResource("/org/apache/avro/ipc/stats/static"));
+ 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 });
Review Comment:
this can be simplified to:
```suggestion
HandlerList handlers = new HandlerList(staticContext, servletContext);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
