-
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-22013 SpaceQuotas - getNumRegions() returning wrong number of regions due to region replicas #570
Conversation
💔 -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. |
@@ -764,6 +765,9 @@ int getNumRegions(TableName table) throws IOException { | |||
List<RegionInfo> regions = this.conn.getAdmin().getRegions(table); | |||
if (regions == null) { | |||
return 0; | |||
} else { |
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: the else
branch is unnecessary. Just make this if (regions==null) {return 0;} RegionReplicaUtil...
(but properly formatted).
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.
Hi @joshelser , Thanks for the review. Did all the changes and pushed. :)
p.addColumn(Bytes.toBytes(SpaceQuotaHelperForTests.F1), Bytes.toBytes("to"), | ||
Bytes.toBytes("reject")); | ||
// Adding a sleep for 5 sec, so all the chores run and to void flakiness of the test. | ||
Thread.sleep(5000); |
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.
👎 Make this an explicit check. What do you need to propagate so that this test can pass? The RegionServerQuotaManager needs to be aware of the quota that was set? You can look at that via HBaseTestingUtility
and the HRegionServer
object you can get from there.
Is that clear? Like your other PR, there should be examples of doing this already in the o.a.h.h.quotas
package.
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.
Not required here since verifyViolation() has NUM_RETRIES as 10 and also has some wait. So removing the Thread.Sleep here. Even without it, testcase will always pass.
|
||
@Test | ||
public void testSetQuotaWithRegionReplicaMultipleRegion() throws Exception { | ||
setQuotaAndVerifyForRegionReplication(5, 3, SpaceViolationPolicy.NO_INSERTS); |
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.
Is it intentional that this is 5
and the other lines are 6
?
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.
Not intentional. changed it to 6 only.
|
||
@Test | ||
public void testSetQuotaWithRegionReplicaSingleRegion() throws Exception { | ||
setQuotaAndVerifyForRegionReplication(1, 2, SpaceViolationPolicy.NO_INSERTS); |
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.
Turn this into a for-loop with SpaceViolationPolicy.values()
?
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.
Done
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 for the fixing this up. Will wait for QA to come back before committing, but otherwise looks fine.
💔 -1 overall
This message was automatically generated. |
Looks like the new test (and all of the old quota tests) passed. Will get this in. |
Thank you @joshelser :-) |
…ons for a table Closes #570 Signed-off-by: Josh Elser <elserj@apache.org>
…ons for a table Closes #570 Signed-off-by: Josh Elser <elserj@apache.org>
…ons for a table Closes #570 Signed-off-by: Josh Elser <elserj@apache.org>
…ons for a table Closes apache#570 Signed-off-by: Josh Elser <elserj@apache.org>
…ons for a table Closes apache#570 Signed-off-by: Josh Elser <elserj@apache.org> (cherry picked from commit 973ec21) Change-Id: I1a9dd12df50867672042e7cf4738ea94896f6085
Space Quota: Space Quota Issue: If a table is created with region replica then quota calculation is not happening
Steps:
1: Create a table with 100 regions with region replica 3
2: Observe that 'hbase:quota' table doesn't have entry of usage for this table So In UI only policy Limit and Policy is shown but not Usage and State.
Reason:
It looks like File system utilization core is sending data of 100 reiogns but not the size of region replicas.
But in quota observer chore, it is considering total region(actual regions+ replica reasons)
So the ratio of reported regions is less then configured percentRegionsReportedThreshold.
So quota calculation is not happening