-
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-22527 [hbck2] Add a master web ui to show the problematic regions #373
Conversation
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.
Add a UT?
@@ -23,9 +23,12 @@ | |||
import java.util.Collections; | |||
import java.util.HashMap; | |||
import java.util.HashSet; | |||
import java.util.LinkedList; |
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's this...
@@ -158,6 +161,8 @@ | |||
private final RegionStates regionStates = new RegionStates(); | |||
private final RegionStateStore regionStateStore; | |||
|
|||
private final ConcurrentMap<ServerName, Set<byte[]>> rsReports = new ConcurrentHashMap<>(); |
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 looks like all of our access is synchronized, why are we using CHM?
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.
Changed to HashMap in new patch.
@@ -2028,4 +2036,41 @@ private void addToPendingAssignment(final HashMap<RegionInfo, RegionStateNode> r | |||
MasterServices getMaster() { | |||
return master; | |||
} | |||
|
|||
public Map<byte[], Pair<ServerName, Set<ServerName>>> getProblematicRegions() { |
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 do we get from this method to the master UI?
we need a way to get the same information via a cli, either hbck1 or shell.
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.
Added the UI in new patch.
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 thought hbck1 already have the ability to find these problematic regions?
@@ -2028,4 +2036,41 @@ private void addToPendingAssignment(final HashMap<RegionInfo, RegionStateNode> r | |||
MasterServices getMaster() { | |||
return master; | |||
} | |||
|
|||
public Map<byte[], Pair<ServerName, Set<ServerName>>> getProblematicRegions() { |
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 method needs a javadoc description of what we're returning
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.
Added the javadoc in new patch.
@@ -2028,4 +2036,41 @@ private void addToPendingAssignment(final HashMap<RegionInfo, RegionStateNode> r | |||
MasterServices getMaster() { | |||
return master; | |||
} | |||
|
|||
public Map<byte[], Pair<ServerName, Set<ServerName>>> getProblematicRegions() { | |||
ConcurrentMap<byte[], Set<ServerName>> reportedOnlineRegions = new ConcurrentHashMap<>(); |
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.
why is this a concurrent map?
💔 -1 overall
This message was automatically generated. |
7d237eb
to
4b5243f
Compare
💔 -1 overall
This message was automatically generated. |
bc945a1
to
0b6fc5d
Compare
💔 -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.
Really great. Just a note on something to add on commit.
int numOfPages = (int) Math.ceil(totalSize * 1.0 / sizePerPage); | ||
</%java> | ||
<section> | ||
<h2><a name="rit">Problematic Regions</a></h2> |
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.
On commit, add a sentence that says what a problematic region is. It seems like its one that has a meta entry that does not agree w/ where it is actually deployed?
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.
Or, you say below what it is... * case 1. Master thought this region opened, but no regionserver reported it.
- case 2. Master thought this region opened on Server1, but regionserver reported Server2
- case 3. More than one regionservers reported opened this region
Can this be added in the UI maybe in small text?
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.
Already committed it. Will add addendum for this.
No description provided.