HBASE-29596 Migrate Canary Status Jamon page back to JSP #7390
+383
−771
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
Contributor
Author
TestingTested this by starting HBase in standalone mode with Configured Canary tool info port in ...
<property>
<name>hbase.canary.info.port</name>
<value>16019</value>
</property>
...
Started the Canary tool in daemon mode (this provides the Canary Status page web UI) with Checked the Canary Status web UI page: 
|
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
TestRSGroupsWithACL failed in the PR build. I think it is not related. |
There was a problem hiding this comment.
Pull Request Overview
This PR removes the Jamon template dependency from HBase and replaces the Jamon-based Canary status page with a native JSP implementation. The key changes include removing Jamon dependencies, converting the Jamon template to JSP, creating a utility class for shared functionality, and adding comprehensive test coverage for the new JSP-based web UI.
- Removes all Jamon dependencies and build configurations across multiple modules
- Replaces Jamon template (
CanaryStatusTmpl.jamon) with a native JSP implementation (canary.jsp) - Creates a new utility class
CanaryStatusUtilfor generating server name links - Adds comprehensive test coverage for the Canary web UI functionality
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Removes Jamon dependency and plugin version properties from parent POM |
| hbase-server/pom.xml | Removes Jamon runtime dependency, plugin configuration, and Eclipse plugin settings for Jamon; adds JSP compilation for canary webapp |
| hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/tool/CanaryStatusTmpl.jamon | Deletes the Jamon template file |
| hbase-server/src/main/resources/hbase-webapps/canary/canary.jsp | Replaces redirect with full JSP implementation of Canary status page |
| hbase-server/src/main/java/org/apache/hadoop/hbase/tool/CanaryStatusServlet.java | Simplifies servlet to only redirect to canary.jsp instead of rendering Jamon template |
| hbase-server/src/main/java/org/apache/hadoop/hbase/util/CanaryStatusUtil.java | Adds new utility class for generating server name links |
| hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestCanaryStatusUtil.java | Adds tests for the new utility class |
| hbase-server/src/test/java/org/apache/hadoop/hbase/tool/TestCanaryStatusServlet.java | Removes tests for Jamon-based servlet |
| hbase-server/src/test/java/org/apache/hadoop/hbase/tool/TestCanaryTool.java | Adds comprehensive integration tests for the new JSP-based web UI |
| hbase-zookeeper/pom.xml | Removes Jamon Eclipse plugin configuration |
| hbase-shaded/pom.xml | Removes Jamon package relocation configuration |
| hbase-resource-bundle/src/main/resources/supplemental-models.xml | Removes Jamon license information |
| hbase-checkstyle/src/main/resources/hbase/checkstyle-suppressions.xml | Removes checkstyle suppression for generated Jamon files |
| src/main/asciidoc/_chapters/developer.adoc | Updates documentation to remove reference to generated-jamon folder |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hbase-server/src/main/resources/hbase-webapps/canary/canary.jsp
Outdated
Show resolved
Hide resolved
Comment on lines
+34
to
+40
| int infoPort = serverName.getPort() + 1; | ||
| String url = "//" + serverName.getHostname() + ":" + infoPort + "/"; | ||
| if (infoPort > 0) { | ||
| return "<a href=\"" + url + "\">" + serverName.getServerName() + "</a>"; | ||
| } else { | ||
| return serverName.getServerName(); | ||
| } |
There was a problem hiding this comment.
The logic is incorrect: when serverName.getPort() returns -1, infoPort becomes 0, but the condition checks if (infoPort > 0). This means when the port is -1, infoPort is 0, which fails the > 0 check correctly. However, when the port is valid (e.g., 12345), infoPort is 12346 (always > 0), so the link is always returned. The condition should check if the original port is valid (>= 0) before creating the link, not if infoPort > 0. The correct check should be if (serverName.getPort() >= 0) or if (infoPort > 0) should be if (serverName.getPort() >= 0).
Contributor
Author
There was a problem hiding this comment.
This method was actually a Jamon sub-template which I converted to Java (as close as possible) and added unit tests.
I think it is correct as is because when serverName.getPort() returns -1 (invalid port), infoPort becomes 0 and then we only return serverName.getServerName(). Otherwise we use the the port to build the URL.
But please correct me if I'm wrong.
hbase-server/src/test/java/org/apache/hadoop/hbase/tool/TestCanaryTool.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
PDavid
force-pushed
the
HBASE-29596-jamon-jsp-canary
branch
from
502510a to
0244319
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the last step of the Jamon to JSP migration: the Canary Status page.
Did the migration the same way as for the:
Migrated the Jamon code to JSP as close as possible. Extracted some duplicated server link code to new java class: CanaryStatusUtil and added unit tests.
Changed the Canary Server Status page back to
/canary.jsp. Made sure that/canary-statusredirects to/canary.jsp.Introduced a
src/main/resources/hbase-webapps/commondirectory where we can place common JSP files which are used by both Master and RegionServer JSP pages. This required to adjust the JSP compiler Maven Antrun plugin a bit.Made sure to compile Canary JSP files the same way as we do for Master and Region server JSP pages.
Removed the Jamon Maven dependencies and references to Jamon code.