Copilot commented on code in PR #8256:
URL: https://github.com/apache/hbase/pull/8256#discussion_r3263728973
##########
hbase-server/src/main/resources/hbase-webapps/regionserver/region.jsp:
##########
@@ -95,7 +100,51 @@
<p> <%= count %> StoreFile(s) in set. <%= isReplicaRegion ? "The
information about storefile(s) may not up-to-date because it's not the primary
region." : "" %></p>
</table>
- <% }
+
+ <% if (store instanceof HMobStore) { %>
+ <h4>MOB Files</h4>
+ <table class="table table-striped">
+ <tr>
+ <th>MOB File</th>
+ <th>Size (MB)</th>
+ <th>Modification time</th>
+ </tr>
+
+ <%
+ int mobCnt = 0;
+ for (HStoreFile sf : storeFiles) {
+ try {
+ byte[] value = sf.getMetadataValue(HStoreFile.MOB_FILE_REFS);
+ if (value == null) {
+ continue;
+ }
+
+ Collection<String> fileNames =
MobUtils.deserializeMobFileRefs(value).build().values();
+ mobCnt += fileNames.size();
+ for (String fileName : fileNames) {
+ Path mobPath = new Path(((HMobStore) store).getPath(),
fileName);
+ FileStatus status = fs.getFileStatus(mobPath);
Review Comment:
The MOB file list is built by iterating each HStoreFile’s MOB_FILE_REFS and
printing every referenced MOB filename. The same MOB file can be referenced by
multiple store files, which will lead to duplicate rows and an inflated mobCnt.
Consider deduplicating (e.g., collect into a Set, optionally sort) before
rendering and counting, and then fetch FileStatus once per unique MOB path.
##########
hbase-server/src/main/resources/hbase-webapps/regionserver/region.jsp:
##########
@@ -95,7 +100,51 @@
<p> <%= count %> StoreFile(s) in set. <%= isReplicaRegion ? "The
information about storefile(s) may not up-to-date because it's not the primary
region." : "" %></p>
</table>
- <% }
+
+ <% if (store instanceof HMobStore) { %>
+ <h4>MOB Files</h4>
+ <table class="table table-striped">
+ <tr>
+ <th>MOB File</th>
+ <th>Size (MB)</th>
+ <th>Modification time</th>
+ </tr>
+
+ <%
+ int mobCnt = 0;
+ for (HStoreFile sf : storeFiles) {
+ try {
+ byte[] value = sf.getMetadataValue(HStoreFile.MOB_FILE_REFS);
+ if (value == null) {
+ continue;
+ }
+
+ Collection<String> fileNames =
MobUtils.deserializeMobFileRefs(value).build().values();
+ mobCnt += fileNames.size();
+ for (String fileName : fileNames) {
+ Path mobPath = new Path(((HMobStore) store).getPath(),
fileName);
+ FileStatus status = fs.getFileStatus(mobPath);
+ String mobPathStr = mobPath.toString();
+ String encodedStr = URLEncoder.encode(mobPathStr,
StandardCharsets.UTF_8.toString()); %>
+
+ <tr>
+ <td><a href="storeFile.jsp?name=<%= encodedStr %>"><%=
mobPathStr %></a></td>
+ <td><%= status.getLen() / 1024 / 1024 %></td>
+ <td><%= new Date(status.getModificationTime()) %></td>
Review Comment:
In the MOB files loop, we call fs.getFileStatus(mobPath) unconditionally.
For replica regions, the store-file section skips files that are not present on
the local FS (fs.exists(...)); the MOB section should do the same (or otherwise
handle missing MOB files) to avoid throwing FileNotFoundException and rendering
it to the page.
##########
hbase-server/src/main/resources/hbase-webapps/regionserver/region.jsp:
##########
@@ -95,7 +100,51 @@
<p> <%= count %> StoreFile(s) in set. <%= isReplicaRegion ? "The
information about storefile(s) may not up-to-date because it's not the primary
region." : "" %></p>
</table>
- <% }
+
+ <% if (store instanceof HMobStore) { %>
+ <h4>MOB Files</h4>
+ <table class="table table-striped">
+ <tr>
+ <th>MOB File</th>
+ <th>Size (MB)</th>
+ <th>Modification time</th>
+ </tr>
+
+ <%
+ int mobCnt = 0;
+ for (HStoreFile sf : storeFiles) {
+ try {
+ byte[] value = sf.getMetadataValue(HStoreFile.MOB_FILE_REFS);
+ if (value == null) {
+ continue;
+ }
+
+ Collection<String> fileNames =
MobUtils.deserializeMobFileRefs(value).build().values();
+ mobCnt += fileNames.size();
+ for (String fileName : fileNames) {
+ Path mobPath = new Path(((HMobStore) store).getPath(),
fileName);
+ FileStatus status = fs.getFileStatus(mobPath);
+ String mobPathStr = mobPath.toString();
+ String encodedStr = URLEncoder.encode(mobPathStr,
StandardCharsets.UTF_8.toString()); %>
+
+ <tr>
+ <td><a href="storeFile.jsp?name=<%= encodedStr %>"><%=
mobPathStr %></a></td>
+ <td><%= status.getLen() / 1024 / 1024 %></td>
+ <td><%= new Date(status.getModificationTime()) %></td>
+ </tr>
+
+ <% }
+ } catch (Exception e) { %>
+ <tr>
+ <td colspan="3"><%= e %></td>
+ </tr>
+ <% }
Review Comment:
Catching a generic Exception and printing it directly into the HTML table
leaks internal details (paths, stack context in the message) and makes the UI
noisy. Prefer catching expected exceptions (e.g.,
FileNotFoundException/IOException), logging server-side, and rendering a short,
user-friendly message instead of dumping the exception object.
##########
hbase-server/src/main/resources/hbase-webapps/regionserver/region.jsp:
##########
@@ -95,7 +100,51 @@
<p> <%= count %> StoreFile(s) in set. <%= isReplicaRegion ? "The
information about storefile(s) may not up-to-date because it's not the primary
region." : "" %></p>
</table>
- <% }
+
+ <% if (store instanceof HMobStore) { %>
+ <h4>MOB Files</h4>
+ <table class="table table-striped">
+ <tr>
+ <th>MOB File</th>
+ <th>Size (MB)</th>
+ <th>Modification time</th>
+ </tr>
+
+ <%
+ int mobCnt = 0;
+ for (HStoreFile sf : storeFiles) {
+ try {
+ byte[] value = sf.getMetadataValue(HStoreFile.MOB_FILE_REFS);
+ if (value == null) {
+ continue;
+ }
+
+ Collection<String> fileNames =
MobUtils.deserializeMobFileRefs(value).build().values();
+ mobCnt += fileNames.size();
+ for (String fileName : fileNames) {
+ Path mobPath = new Path(((HMobStore) store).getPath(),
fileName);
+ FileStatus status = fs.getFileStatus(mobPath);
+ String mobPathStr = mobPath.toString();
+ String encodedStr = URLEncoder.encode(mobPathStr,
StandardCharsets.UTF_8.toString()); %>
+
+ <tr>
+ <td><a href="storeFile.jsp?name=<%= encodedStr %>"><%=
mobPathStr %></a></td>
+ <td><%= status.getLen() / 1024 / 1024 %></td>
+ <td><%= new Date(status.getModificationTime()) %></td>
+ </tr>
+
+ <% }
+ } catch (Exception e) { %>
+ <tr>
+ <td colspan="3"><%= e %></td>
+ </tr>
+ <% }
+ } %>
+
+ <p> <%= mobCnt %> MobFile(s) in set.</p>
+ </table>
Review Comment:
The summary paragraph ("<%= mobCnt %> MobFile(s) in set") is nested directly
inside the <table>, which is invalid HTML (tables should contain
caption/thead/tbody/tfoot/tr). Move the summary outside the table (or use a
<caption>/<tfoot>) to keep the markup valid and consistent with HTML semantics.
--
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]
