Skip to content

HBASE-22013 SpaceQuotas - getNumRegions() returning wrong number of regions due to region replicas #570

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.

Sign up for GitHub

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

Closed
shardul-cr7 wants to merge 4 commits into from
Closed

HBASE-22013 SpaceQuotas - getNumRegions() returning wrong number of regions due to region replicas #570

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24725e4
HBASE-22013
shardul-cr7 Sep 1, 2019
0309bcf
checkstyle
shardul-cr7 Sep 3, 2019
5260281
asf warning
shardul-cr7 Sep 3, 2019
bf59f9c
review comments handled
shardul-cr7 Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaObserverChore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.client.Connection;
import org.apache.hadoop.hbase.client.RegionInfo;
import org.apache.hadoop.hbase.client.RegionReplicaUtil;
import org.apache.hadoop.hbase.client.Scan;
import org.apache.hadoop.hbase.master.HMaster;
import org.apache.hadoop.hbase.master.MetricsMaster;
Expand Down Expand Up @@ -764,6 +765,9 @@ int getNumRegions(TableName table) throws IOException {
List<RegionInfo> regions = this.conn.getAdmin().getRegions(table);
if (regions == null) {
return 0;
} else {
Copy link
Member

@joshelser joshelser Sep 4, 2019

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).

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 6, 2019

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. :)

// Filter the region replicas if any and return the original number of regions for a table.
RegionReplicaUtil.removeNonDefaultRegions(regions);
}
return regions.size();
}
Expand Down
19 changes: 13 additions & 6 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/SpaceQuotaHelperForTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,15 +477,16 @@ TableName createTableWithRegions(int numRegions) throws Exception {
}

TableName createTableWithRegions(Admin admin, int numRegions) throws Exception {
return createTableWithRegions(
testUtil.getAdmin(), NamespaceDescriptor.DEFAULT_NAMESPACE_NAME_STR, numRegions);
return createTableWithRegions(admin, NamespaceDescriptor.DEFAULT_NAMESPACE_NAME_STR, numRegions,
0);
}

TableName createTableWithRegions(String namespace, int numRegions) throws Exception {
return createTableWithRegions(testUtil.getAdmin(), namespace, numRegions);
return createTableWithRegions(testUtil.getAdmin(), namespace, numRegions, 0);
}

TableName createTableWithRegions(Admin admin, String namespace, int numRegions) throws Exception {
TableName createTableWithRegions(Admin admin, String namespace, int numRegions,
int numberOfReplicas) throws Exception {
final TableName tn = getNextTableName(namespace);

// Delete the old table
Expand All @@ -495,8 +496,14 @@ TableName createTableWithRegions(Admin admin, String namespace, int numRegions)
}

// Create the table
TableDescriptor tableDesc = TableDescriptorBuilder.newBuilder(tn)
.setColumnFamily(ColumnFamilyDescriptorBuilder.of(F1)).build();
TableDescriptor tableDesc;
if (numberOfReplicas > 0) {
tableDesc = TableDescriptorBuilder.newBuilder(tn).setRegionReplication(numberOfReplicas)
.setColumnFamily(ColumnFamilyDescriptorBuilder.of(F1)).build();
} else {
tableDesc = TableDescriptorBuilder.newBuilder(tn)
.setColumnFamily(ColumnFamilyDescriptorBuilder.of(F1)).build();
}
if (numRegions == 1) {
admin.createTable(tableDesc);
} else {
Expand Down
119 changes: 119 additions & 0 deletions ...erver/src/test/java/org/apache/hadoop/hbase/quotas/TestSpaceQuotasWithRegionReplicas.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.hadoop.hbase.quotas;

import java.util.concurrent.atomic.AtomicLong;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HBaseTestingUtility;
import org.apache.hadoop.hbase.NamespaceDescriptor;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.client.Put;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.util.Bytes;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.experimental.categories.Category;
import org.junit.rules.TestName;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;


@Category(MediumTests.class)
public class TestSpaceQuotasWithRegionReplicas {

@ClassRule
public static final HBaseClassTestRule CLASS_RULE =
HBaseClassTestRule.forClass(TestSpaceQuotasWithRegionReplicas.class);

private static final Logger LOG =
LoggerFactory.getLogger(TestSpaceQuotasWithRegionReplicas.class);
private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();
private static final int NUM_RETRIES = 10;

@Rule
public TestName testName = new TestName();
private SpaceQuotaHelperForTests helper;

@BeforeClass
public static void setUp() throws Exception {
Configuration conf = TEST_UTIL.getConfiguration();
SpaceQuotaHelperForTests.updateConfigForQuotas(conf);
TEST_UTIL.startMiniCluster(1);
}

@AfterClass
public static void tearDown() throws Exception {
TEST_UTIL.shutdownMiniCluster();
}

@Before
public void removeAllQuotas() throws Exception {
helper = new SpaceQuotaHelperForTests(TEST_UTIL, testName, new AtomicLong(0));
helper.removeAllQuotas();
}

@Test
public void testSetQuotaWithRegionReplicaSingleRegion() throws Exception {
setQuotaAndVerifyForRegionReplication(1, 2, SpaceViolationPolicy.NO_INSERTS);
Copy link
Member

@joshelser joshelser Sep 4, 2019

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()?

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

setQuotaAndVerifyForRegionReplication(1, 2, SpaceViolationPolicy.NO_WRITES);
setQuotaAndVerifyForRegionReplication(1, 2, SpaceViolationPolicy.NO_WRITES_COMPACTIONS);
setQuotaAndVerifyForRegionReplication(1, 2, SpaceViolationPolicy.DISABLE);
}

@Test
public void testSetQuotaWithRegionReplicaMultipleRegion() throws Exception {
setQuotaAndVerifyForRegionReplication(5, 3, SpaceViolationPolicy.NO_INSERTS);
Copy link
Member

@joshelser joshelser Sep 4, 2019

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?

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 6, 2019

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.

setQuotaAndVerifyForRegionReplication(6, 3, SpaceViolationPolicy.NO_WRITES);
setQuotaAndVerifyForRegionReplication(6, 3, SpaceViolationPolicy.NO_WRITES_COMPACTIONS);
setQuotaAndVerifyForRegionReplication(6, 3, SpaceViolationPolicy.DISABLE);
}

@Test
public void testSetQuotaWithSingleRegionZeroRegionReplica() throws Exception {
setQuotaAndVerifyForRegionReplication(1, 0, SpaceViolationPolicy.NO_INSERTS);
setQuotaAndVerifyForRegionReplication(1, 0, SpaceViolationPolicy.NO_WRITES);
setQuotaAndVerifyForRegionReplication(1, 0, SpaceViolationPolicy.NO_WRITES_COMPACTIONS);
setQuotaAndVerifyForRegionReplication(1, 0, SpaceViolationPolicy.DISABLE);
}

@Test
public void testSetQuotaWithMultipleRegionZeroRegionReplicas() throws Exception {
setQuotaAndVerifyForRegionReplication(5, 0, SpaceViolationPolicy.NO_INSERTS);
setQuotaAndVerifyForRegionReplication(6, 0, SpaceViolationPolicy.NO_WRITES);
setQuotaAndVerifyForRegionReplication(6, 0, SpaceViolationPolicy.NO_WRITES_COMPACTIONS);
setQuotaAndVerifyForRegionReplication(6, 0, SpaceViolationPolicy.DISABLE);
}

private void setQuotaAndVerifyForRegionReplication(int region, int replicatedRegion,
SpaceViolationPolicy policy) throws Exception {
TableName tn = helper.createTableWithRegions(TEST_UTIL.getAdmin(),
NamespaceDescriptor.DEFAULT_NAMESPACE_NAME_STR, region, replicatedRegion);
helper.setQuotaLimit(tn, policy, 5L);
helper.writeData(tn, 5L * SpaceQuotaHelperForTests.ONE_MEGABYTE);
Put p = new Put(Bytes.toBytes("to_reject"));
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);
Copy link
Member

@joshelser joshelser Sep 4, 2019

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.

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 6, 2019

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.

helper.verifyViolation(policy, tn, p);
}
}