Copilot commented on code in PR #10847:
URL: https://github.com/apache/gravitino/pull/10847#discussion_r3128322386
##########
server/src/main/java/org/apache/gravitino/server/web/HealthAliasServlet.java:
##########
@@ -37,8 +37,11 @@ public class HealthAliasServlet extends HttpServlet {
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp)
throws ServletException, IOException {
- // Prepend /api to the incoming path: /health → /api/health, /health/live
→ /api/health/live
- String targetPath = "/api" + req.getRequestURI();
+ // Map root-level health paths to their canonical /api/health counterparts.
+ // /health and /health.html both target the aggregate /api/health; legacy
GTM
+ // standards sometimes hardcode the .html extension.
+ String uri = req.getRequestURI();
+ String targetPath = "/health.html".equals(uri) ? "/api/health" : "/api" +
uri;
Review Comment:
HealthAliasServlet’s class JavaDoc still enumerates only /health,
/health/live, /health/ready, but the implementation now explicitly supports
/health.html as well. Please update the JavaDoc to include /health.html so the
documentation matches the behavior.
##########
server/src/main/java/org/apache/gravitino/server/GravitinoServer.java:
##########
@@ -180,6 +180,7 @@ protected void configure() {
// Root-level alias for enterprise GTMs that require probes at well-known
root paths.
// Forwards /health, /health/live, /health/ready to the canonical
/api/health/* endpoints.
server.addServlet(new HealthAliasServlet(), "/health/*");
+ server.addServlet(new HealthAliasServlet(), "/health.html");
Review Comment:
The bug being fixed here was caused by Jetty path-spec matching
(/health.html wasn’t registered). Current tests only validate the servlet’s
forwarding logic and auth bypass, but don’t exercise GravitinoServer/Jetty
routing, so the same regression could recur unnoticed. Add a small
server-level/integration test that starts the server and asserts GET
/health.html is handled (e.g., returns 200/503 but not 404, and/or matches the
/api/health body).
##########
server/src/main/java/org/apache/gravitino/server/GravitinoServer.java:
##########
@@ -180,6 +180,7 @@ protected void configure() {
// Root-level alias for enterprise GTMs that require probes at well-known
root paths.
// Forwards /health, /health/live, /health/ready to the canonical
/api/health/* endpoints.
Review Comment:
The root-level alias comment is now incomplete: it lists /health,
/health/live, /health/ready but this block also registers /health.html. Please
update the comment to reflect the full set of supported alias paths so future
readers don’t miss the .html mapping.
```suggestion
// Root-level aliases for enterprise GTMs that require probes at
well-known root paths.
// Forwards /health, /health/live, /health/ready, and /health.html to
the canonical
// /api/health/* endpoints.
```
--
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]
