You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/05/05 16:44:16 UTC
[GitHub] [hbase] ndimiduk commented on a change in pull request #1485: HBASE-23969 Meta browser should show all `info` columns
ndimiduk commented on a change in pull request #1485:
URL: https://github.com/apache/hbase/pull/1485#discussion_r420238552
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/webapp/RegionReplicaInfo.java
##########
@@ -17,14 +17,22 @@
*/
package org.apache.hadoop.hbase.master.webapp;
+import static org.apache.hbase.thirdparty.org.apache.commons.collections4.ListUtils.emptyIfNull;
Review comment:
A few unused imports to cleanup.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java
##########
@@ -26,6 +26,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
+import java.util.HashMap;
Review comment:
unused import.
##########
File path: hbase-server/src/main/resources/hbase-webapps/master/table.jsp
##########
@@ -387,68 +388,93 @@ if (fqtn != null && master.isInitialized()) {
}
}
%>
-<table class="table table-striped">
- <tr>
- <th>RegionName</th>
- <th>Start Key</th>
- <th>End Key</th>
- <th>Replica ID</th>
- <th>RegionState</th>
- <th>ServerName</th>
- </tr>
-<%
- final boolean metaScanHasMore;
- byte[] lastRow = null;
- try (final MetaBrowser.Results results = metaBrowser.getResults()) {
- for (final RegionReplicaInfo regionReplicaInfo : results) {
- lastRow = Optional.ofNullable(regionReplicaInfo)
- .map(RegionReplicaInfo::getRow)
- .orElse(null);
- if (regionReplicaInfo == null) {
-%>
- <tr>
- <td colspan="6">Null result</td>
- </tr>
-<%
- continue;
- }
+<div style="overflow-x: auto">
+ <table class="table table-striped nowrap">
+ <tr>
+ <th title="Region name">RegionName</th>
+ <th title="The startKey of this region">Start Key</th>
+ <th title="The endKey of this region">End Key</th>
+ <th title="Region replica id">Replica ID</th>
+ <th title="State of the region while undergoing transitions">RegionState</th>
+ <th title="Server hosting this region replica, stored in info:server column">Server</th>
Review comment:
It's a bit tedious, but how about we build all these labels from the values in `HConstants`? That way this page is captured by any future refactoring that happens down the line. For instance, the bit in this tool tip that says "info:server" would be replaced with `+ HConstants.CATALOG_FAMILY_STR + ":" + HConstants.SERVER_QUALIFIER_STR`.
Kind of fiddly. I'm not sure if it's worth it. Just a suggestion.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java
##########
@@ -966,6 +965,33 @@ public static ServerName getServerName(final Result r, final int replicaId) {
}
}
+ /**
+ * Returns the {@link ServerName} from catalog table {@link Result} where the region is
+ * transitioning on. It should be the same as {@link MetaTableAccessor#getServerName(Result,int)}
+ * if the server is at OPEN state.
+ *
+ * @param r Result to pull the transitioning server name from
+ * @return A ServerName instance or {@link MetaTableAccessor#getServerName(Result,int)}
+ * if necessary fields not found or empty.
+ */
+ @Nullable
+ public static ServerName getTargetServerName(final Result r, final int replicaId) {
Review comment:
Long-stale conversation, perhaps...
I agree with @liuml07 on this one -- it's a good thing to have a centralized place for encapsulating the serialization details of storing and retrieving content from meta. `MetaTableAccessor` seems like the place for it.
##########
File path: hbase-server/src/main/resources/hbase-webapps/master/table.jsp
##########
@@ -387,68 +388,93 @@ if (fqtn != null && master.isInitialized()) {
}
}
%>
-<table class="table table-striped">
- <tr>
- <th>RegionName</th>
- <th>Start Key</th>
- <th>End Key</th>
- <th>Replica ID</th>
- <th>RegionState</th>
- <th>ServerName</th>
- </tr>
-<%
- final boolean metaScanHasMore;
- byte[] lastRow = null;
- try (final MetaBrowser.Results results = metaBrowser.getResults()) {
- for (final RegionReplicaInfo regionReplicaInfo : results) {
- lastRow = Optional.ofNullable(regionReplicaInfo)
- .map(RegionReplicaInfo::getRow)
- .orElse(null);
- if (regionReplicaInfo == null) {
-%>
- <tr>
- <td colspan="6">Null result</td>
- </tr>
-<%
- continue;
- }
+<div style="overflow-x: auto">
+ <table class="table table-striped nowrap">
+ <tr>
+ <th title="Region name">RegionName</th>
+ <th title="The startKey of this region">Start Key</th>
+ <th title="The endKey of this region">End Key</th>
+ <th title="Region replica id">Replica ID</th>
+ <th title="State of the region while undergoing transitions">RegionState</th>
+ <th title="Server hosting this region replica, stored in info:server column">Server</th>
+ <th title="The seqNum for the region at the time the server opened this region replica">Sequence Number</th>
+ <th title="Server where the region is transitioning on, stored in info:sn column">Target Server</th>
Review comment:
s/Server where the region is transitioning on/The server to which the region is transiting/
##########
File path: hbase-server/src/main/resources/hbase-webapps/static/css/hbase.css
##########
@@ -54,3 +54,7 @@ table.tablesorter thead tr .headerSortUp {
table.tablesorter thead tr .headerSortDown {
background-image: url(desc.gif);
}
+
+table.nowrap th, td {
Review comment:
Do you intend to change the style of all `td` elements, or only those under `table.nowrap`? If the latter, I think you need `table.nowrap th, table.nowrap td`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org