HBASE-29223 Migrate Master Status Jamon page back to JSP #6875
+3,158
−2,276
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
PDavid
commented
Apr 1, 2025
Comment on lines
+28
to
+35
| public static final String FRAGS = "frags"; | ||
| public static final String SERVER_NAMES = "serverNames"; | ||
| public static final String SERVER_NAME = "serverName"; | ||
| public static final String RS_GROUP_INFOS = "rsGroupInfos"; | ||
| public static final String COLLECT_SERVERS = "collectServers"; | ||
| public static final String FILTER = "filter"; | ||
| public static final String FORMAT = "format"; | ||
| public static final String PARENT = "parent"; |
There was a problem hiding this comment.
Maybe we could just have these constants in MasterStatusUtil and then we don't need a separate class for this. What do you think?
This comment has been minimized.
This comment has been minimized.
PDavid
force-pushed
the
HBASE-29223-jamon-jsp
branch
from
c5216a6 to
e09403a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I tested this locally in standalone mode after building with maven (
I will still have to test these special cases as these have special UI parts:
I will also test with Pseudo-Distributed mode. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Apache9
reviewed
Apr 9, 2025
hbase-server/src/test/java/org/apache/hadoop/hbase/master/http/TestMasterStatusServlet.java
Show resolved
Hide resolved
|
Let me see if the copilot can give some feedbacks since this is a very big change... |
There was a problem hiding this comment.
Copilot reviewed 39 out of 45 changed files in this pull request and generated 3 comments.
Files not reviewed (6)
- hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/AssignmentManagerStatusTmpl.jamon: Language not supported
- hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/BackupMasterStatusTmpl.jamon: Language not supported
- hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/RSGroupListTmpl.jamon: Language not supported
- hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/RegionVisualizerTmpl.jamon: Language not supported
- hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon: Language not supported
- hbase-server/src/main/resources/hbase-webapps/master/index.html: Language not supported
hbase-server/src/main/resources/hbase-webapps/master/peerConfigs.jsp
Outdated
Show resolved
Hide resolved
hbase-server/src/main/resources/hbase-webapps/master/catalogTables.jsp
Outdated
Show resolved
Hide resolved
Comment on lines
96
to
97
| rsGroupName = server2GroupMap.get(serverName.getAddress()).getName(); | ||
| } |
There was a problem hiding this comment.
This line assumes that server2GroupMap.get(serverName.getAddress()) is non-null. Add a null check before calling getName() to avoid a possible NullPointerException.
Suggested change
| rsGroupName = server2GroupMap.get(serverName.getAddress()).getName(); | |
| } | |
| RSGroupInfo groupInfo = server2GroupMap.get(serverName.getAddress()); | |
| if (groupInfo != null) { | |
| rsGroupName = groupInfo.getName(); | |
| } else { | |
| rsGroupName = "default"; | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
To be honest I'm not sure if this is a good suggestion.
What do you all think?
- getUserTables - getFragmentationInfo - getMetaLocationOrNull
as it is one line and not complex.
…tusUtil as they are only static helper classes.
Added missing slash.
so that we test that `/master-status` redirects to `/master.jsp`.
…mon after rebase they were brought back while resolving conflicts.
- HBASE-28388 Avoid index based field sorting in tablesorter apache#6779 - HBASE-27802 Manage static javascript resources programatically apache#6864
PDavid
force-pushed
the
HBASE-29223-jamon-jsp
branch
from
396d471 to
2464b4f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
PDavid
commented
May 26, 2025
Comment on lines
+103
to
+117
| String hostname = master.getServerName().getHostname(); | ||
| assertTrue(page.contains("<h1>Master <small>" + hostname + "</small></h1>")); | ||
| assertTrue(page.contains("<h2><a name=\"regionservers\">Region Servers</a></h2>")); | ||
| assertRegionServerLinks(master, page); | ||
|
|
||
| assertTrue(page.contains("<h2>Backup Masters</h2>")); | ||
| assertTrue(page.contains("<h2><a name=\"tables\">Tables</a></h2>")); | ||
| assertTableLinks(master, page); | ||
|
|
||
| assertTrue(page.contains("<h2><a name=\"region_visualizer\"></a>Region Visualizer</h2>")); | ||
| assertTrue(page.contains("<h2><a name=\"peers\">Peers</a></h2>")); | ||
| assertTrue(page.contains("<h2><a name=\"tasks\">Tasks</a></h2>")); | ||
| assertTrue(page.contains("<h2><a name=\"attributes\">Software Attributes</a></h2>")); | ||
|
|
||
| assertTrue(page.contains(VersionInfo.getVersion())); |
There was a problem hiding this comment.
For now I only added these assertions on the HTML content of the Master Status page. But we can add more sophisticated asserts in the future.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 first step of the Jamon to JSP migration is the Master Status page.
Moved the Jamon code back to JSP. Changed the Jamon syntax back to JSP.
To transfer data between JSP pages I used request attributes.
Tried to preserve the code as much as possible but did some changes:
Sub-templates were usually extracted to separate JSP file (and included with
<jsp:include), in some case it was extracted as Java method.Extracted some sections from master page to separate JSP pages:
Extracted the long JavaScript from the master page which executes on page load to separate JS file.
Extracted some frequently used static methods to a new util class:
MasterStatusUtil. Also added unit tests for the static methods inMasterStatusUtil.Changed the Master Status page back to
/master.jspagain. Now made sure that/master-statusredirects to/master.jsp.What do you think about this?
I'll create a new Jira for the rest of the Jamon files.