jerryshao commented on code in PR #10847:
URL: https://github.com/apache/gravitino/pull/10847#discussion_r3128339124
##########
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:
Fixed. Updated the inline comment to list all four alias paths: `/health`,
`/health/live`, `/health/ready`, and `/health.html`.
##########
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:
Fixed. Updated the class Javadoc to include `/health.html` in the list of
served root-level paths.
##########
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 Jetty path-spec registration is already covered indirectly by
`TestGravitinoServer.testStartAndStop()`, which starts a real Jetty instance
and calls `stop()`. A dedicated routing integration test would require spinning
up a full Gravitino server and firing HTTP requests, which is the pattern for
the Docker-tagged ITs (e.g. `TestGravitinoServer` ITs) rather than unit tests.
Given the low risk of the mapping regressing (a one-line `addServlet` call),
we'd prefer to keep this in the IT suite rather than add a heavyweight test
here. Happy to add it to the IT suite in a follow-up if you feel strongly.
--
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]
