-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-23969 Meta browser should show all info
columns
#1485
Conversation
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good. @liuml07 do you have one sample screenshot of UI? <div style="overflow-x: auto">
this plays role for wide scrolling right?
.map(RegionInfo::getRegionName) | ||
.map(Bytes::toStringBinary) | ||
.collect(Collectors.joining(", ")) | ||
: ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ? :
is good to use but here since we have some good computation, it might be preferable to return empty string if regionInfo is null initially. Better readability? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been simplified with nullable collection. So no ? :
is required here then.
*/ | ||
@Nullable | ||
@InterfaceAudience.Private // for use by RegionReplicaInfo which is used by meta browser | ||
public static String getTransitioningOnServerName(final Result r, final int replicaId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small test case to cover this would be good. Maybe in TestMaster
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I navigate the source code tree, and found another place has implemented the same logic there. So in recent commit I simply move the code to MetaTableAccessor.java
so it can be reused. I hope I have not broken any stuff, so wait for the QA to confirm. Since that is exactly the existing method, I guess a test is not required? But I can work on a UT if you still suggest. Thanks,
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@virajjasani Yes exactly. I think the table needs a container for horizontal scroll bar so I have to put it in a div. When I filed the PR, I put a screenshot in the JIRA. Should I upload it here as well? |
Oops, my bad, missed looking into Jira attachments. Just saw screenshots, looks all good :) |
Recent commit addresses review comments, specially:
I have also attached the recent screenshot in the JIRA. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Thanks for working on this. Some notes in below. Wonder why column for splitA and for splitB but all regions for merge go into one cell?
* if necessary fields not found or empty. | ||
*/ | ||
@Nullable | ||
public static ServerName getTargetServerName(final Result r, final int replicaId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is rationale for moving this method? How is it MetaTableAccessor material?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. and not private to RegoinStateStore where it was originally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saintstack This method is used so far by both RegionStateStore
and RegionReplicaInfo
(new for table.jsp). So it can not be private to RegoinStateStore. We can keep it there and make it public, or we move to MetaTableAccessor
which is more generic place for Read/write hbase:meta operations. I see all getXXColumn
and getXX
stuff are static methods in MetaTableAccessor which are similar to what this method does. So I moved the logic reading "info:sn" columns to MetaTableAccessor
.
This also came from my coding experience. When I write the first patch, I implemented my version in MetaTableAccessor instead of calling the implementation in RegoinStateStore. I simply did not know existing logic is already someplace there. Meanwhile, both RegionStateStore
and MetaTableAccessor
have their own getServerNameColumn()
method. I guess someone previously may have missed existing implementation as I did :) So overall I prefer the central place of getting columns and data from hbase:meta. But I understand this move makes it public in client module. If that is a concern, I can move it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
<th>Target ServerName</th> | ||
<th>Merge RegionNames</th> | ||
<th>SplitA</th> | ||
<th>SplitB</th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How we do merge? Merge names can be more merge001, merge002... we print them all out in Merge RegionNames? If so, why split different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes merge is all columns matching regex "info:merge*" in hbase:meta. Do we need to know the column name? For "info:split*" I think we can merge A/B into one item as well. I'll make the change as discussion in the JIRA.
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Test failures can pass locally, and seem unrelated and one also fails in other pre-commit builds (e.g. PR-1563). Need to check why. |
1. transitioningOnServerName -> targetServerName, clearer and shorter 2. Reuse existing getServerNameColumn and getRegionServer 3. Add seqNum 4. Merge (parent) region names joined by newline instead of , 5. Use table style instead of repeating th/td style for nowrap
- Rename "ServerName" to "Server", keep "Target Server" for info:sn - Show "info:split*" together with newline-delimited list - Rename "Merger ServerName" to "info:merge*" - Rename SplitA/SplitB columns to "info:split*" - Add tooltip for each column in table head
I merged from |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nits and other suggestions, but I think this is basically ready to go! I like your horizontally scrolling the table without having to scroll the whole page, nice one :)
* if necessary fields not found or empty. | ||
*/ | ||
@Nullable | ||
public static ServerName getTargetServerName(final Result r, final int replicaId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -26,6 +26,7 @@ | |||
import java.util.Arrays; | |||
import java.util.Collection; | |||
import java.util.Collections; | |||
import java.util.HashMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused import.
@@ -17,14 +17,22 @@ | |||
*/ | |||
package org.apache.hadoop.hbase.master.webapp; | |||
|
|||
import static org.apache.hbase.thirdparty.org.apache.commons.collections4.ListUtils.emptyIfNull; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few unused imports to cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I have fixed this.
p.s. For a PR to Hadoop, I usually check the "checkstyle" report by Yetus and update the patch accordingly. I see the Yetus for HBase QA does not report "checkstyle" warnings in the comment table. I found there are report links in the "Console output" though. I'll follow there next time. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to complications around multi-jdk support, we have Yetus run in 3 different invocations. Looks like the checkstyle check gave you a +1.
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Server where the region is transitioning on/The server to which the region is transiting/
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea. I have changed that. To make this table header fit in one line, I have defined some column name variables, which can be reused.
@@ -54,3 +54,7 @@ table.tablesorter thead tr .headerSortUp { | |||
table.tablesorter thead tr .headerSortDown { | |||
background-image: url(desc.gif); | |||
} | |||
|
|||
table.nowrap th, td { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right! No I was going to add the "nowrap" class only and changing default td style may make other places unhappy.
Thanks @ndimiduk I have pushed a new commit to address the comments. I tested locally again and it (still) seems good. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
This looks good to merge. @saintstack any other concerns with the changes to |
Merged to |
Thank you @ndimiduk for helping with this code change! No problem, I'll file a separate PR for |
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
https://issues.apache.org/jira/browse/HBASE-23969