Skip to content

HBASE-22012 Space Quota: DisableTableViolationPolicy will cause cycles of enable/disable table #572

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 3 commits into from
Closed

HBASE-22012 Space Quota: DisableTableViolationPolicy will cause cycles of enable/disable table #572

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e6da835
HBASE-22012
shardul-cr7 Sep 1, 2019
24e2c2f
review comments handled
shardul-cr7 Sep 21, 2019
a635d63
findbugs
shardul-cr7 Sep 22, 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
43 changes: 41 additions & 2 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/MasterQuotaManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

Expand Down Expand Up @@ -702,15 +703,53 @@ int pruneEntriesOlderThan(long timeToPruneBefore) {
Iterator<Entry<RegionInfo,SizeSnapshotWithTimestamp>> iterator =
regionSizes.entrySet().iterator();
while (iterator.hasNext()) {
long currentEntryTime = iterator.next().getValue().getTime();
if (currentEntryTime < timeToPruneBefore) {
RegionInfo regionInfo = iterator.next().getKey();
long currentEntryTime = regionSizes.get(regionInfo).getTime();
boolean isInViolationAndPolicyDisable = isInViolationAndPolicyDisable(regionInfo.getTable());
// do not prune the entries if table is in violation and
// violation policy is disable.prune entries older than time.

@joshelser joshelser Sep 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you buff out this comment with an explanation as to why we need to hold onto RegionSizes for tables in violation with a disable policy or include a reference to HBASE-22012, please?

@shardul-cr7 shardul-cr7 Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

if (currentEntryTime < timeToPruneBefore && !isInViolationAndPolicyDisable) {
iterator.remove();
numEntriesRemoved++;
}
}
return numEntriesRemoved;
}

/**
* Method to check if a table is in violation and policy set on table is DISABLE.
*
* @param tableName tableName to check.
* @return returns true if table is in violation and policy is disable else false.
*/
private boolean isInViolationAndPolicyDisable(TableName tableName) {
boolean isInViolationAtTable = false;
boolean isInViolationAndPolicyDisable = false;
SpaceViolationPolicy policy = null;
try {
if (QuotaUtil.isQuotaEnabled(masterServices.getConfiguration())) {

@joshelser joshelser Sep 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is unnecessary to re-check.

QuotaObserverChore would be calling pruneEntriesOlderThan(long) but that chore is only scheduled if quotas are enabled.

@shardul-cr7 shardul-cr7 Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

// Get Current Snapshot for the given table
SpaceQuotaSnapshot spaceQuotaSnapshot =
QuotaUtil.getCurrentSnapshotFromQuotaTable(masterServices.getConnection(), tableName);

@joshelser joshelser Sep 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you use QuotaObserverChore#getTableQuotaSnapshot(TableName) instead? This is going to hit hbase:quota every time which might be costly.

Since we're inside the master already, we should be able to safely pull from the in-memory state in QuotaObserverChore instead.

@shardul-cr7 shardul-cr7 Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

using QuotaObserverChore#getTableQuotaSnapshot(TableName) now.

if (spaceQuotaSnapshot != null) {
// check if table in violation
isInViolationAtTable = spaceQuotaSnapshot.getQuotaStatus().isInViolation();
Optional<SpaceViolationPolicy> policyAtNamespace =

@joshelser joshelser Sep 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: just policy? AtNamespace doesn't seem to be appropriate.

@shardul-cr7 shardul-cr7 Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

spaceQuotaSnapshot.getQuotaStatus().getPolicy();
if (policyAtNamespace.isPresent()) {
policy = policyAtNamespace.get();
}
}
}
isInViolationAndPolicyDisable =

@joshelser joshelser Sep 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can just make this return policy == SpaceViolationPolicy.DISABLE && isInViolationAtTable;

@joshelser joshelser Sep 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you do this, then you can also remove the variable isInViolationAndPolicyDisable completely, lift the return isInViolationAndPolicyDisable into the catch block and make it return false instead.

@shardul-cr7 shardul-cr7 Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

(policy == SpaceViolationPolicy.DISABLE) && isInViolationAtTable;

} catch (IOException e) {
LOG.info("Problem in getting connection to quota table: ", e);
}
return isInViolationAndPolicyDisable;
}

public void processFileArchivals(FileArchiveNotificationRequest request, Connection conn,
Configuration conf, FileSystem fs) throws IOException {
final HashMultimap<TableName,Entry<String,Long>> archivedFilesByTable = HashMultimap.create();
Expand Down
8 changes: 8 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestMasterQuotaManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,16 @@
*/
package org.apache.hadoop.hbase.quotas;

import static org.apache.hadoop.hbase.quotas.QuotaUtil.QUOTA_CONF_KEY;
import static org.apache.hadoop.hbase.util.Bytes.toBytes;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HBaseTestingUtility;
import org.apache.hadoop.hbase.HRegionInfo;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.master.MasterServices;
Expand Down Expand Up @@ -51,6 +55,10 @@ public void testOldEntriesRemoved() {
MasterServices masterServices = mock(MasterServices.class);
MasterQuotaManager manager = new MasterQuotaManager(masterServices);
manager.initializeRegionSizes();
HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();
Configuration conf = TEST_UTIL.getConfiguration();
conf.set(QUOTA_CONF_KEY, "false");
when(masterServices.getConfiguration()).thenReturn(conf);

@joshelser joshelser Sep 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are you disabling the quota feature here?

@shardul-cr7 shardul-cr7 Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

// Mock out some regions
TableName tableName = TableName.valueOf("foo");
HRegionInfo region1 = new HRegionInfo(tableName, null, toBytes("a"));
Expand Down
30 changes: 30 additions & 0 deletions ...e-server/src/test/java/org/apache/hadoop/hbase/quotas/TestSpaceQuotaBasicFunctioning.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import java.util.Map;
import java.util.concurrent.atomic.AtomicLong;

import org.apache.hadoop.conf.Configuration;
Expand All @@ -31,12 +32,15 @@
import org.apache.hadoop.hbase.client.Delete;
import org.apache.hadoop.hbase.client.Increment;
import org.apache.hadoop.hbase.client.Put;
import org.apache.hadoop.hbase.client.RegionInfo;
import org.apache.hadoop.hbase.client.Table;
import org.apache.hadoop.hbase.master.HMaster;
import org.apache.hadoop.hbase.security.AccessDeniedException;
import org.apache.hadoop.hbase.testclassification.LargeTests;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.util.StringUtils;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.ClassRule;
Expand Down Expand Up @@ -221,4 +225,30 @@ public void testTableQuotaOverridesNamespaceQuota() throws Exception {
Bytes.toBytes("reject"));
helper.verifyViolation(policy, tn, p);
}

@Test
public void testDisablePolicyQuotaAndViolate() throws Exception {
TableName tableName = helper.createTable();
helper.setQuotaLimit(tableName, SpaceViolationPolicy.DISABLE, 2L);
helper.writeData(tableName, SpaceQuotaHelperForTests.ONE_MEGABYTE * 3L);

@joshelser joshelser Sep 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

1MB should be enough to trigger the quota, no?

@shardul-cr7 shardul-cr7 Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed to 1MB.


HMaster master = TEST_UTIL.getMiniHBaseCluster().getMaster();
MasterQuotaManager quotaManager = master.getMasterQuotaManager();

// Sufficient time for all the chores to run.
Thread.sleep(5000);

@joshelser joshelser Sep 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the state you're actually expecting to happen? RegionSize reports to be sent in?

Same issue as one of your recent PRs. Can you please convert this into an wait+explicit check?

@shardul-cr7 shardul-cr7 Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done. added a wait predicate.


long timeToPrune = System.currentTimeMillis() + 11 * 60 * 1000;
quotaManager.pruneEntriesOlderThan(timeToPrune);

// Check if disabled table region report present in the map after retention period expired.
// It should be present after retention period expired.
for (Map.Entry<RegionInfo, Long> entry : quotaManager.snapshotRegionSizes().entrySet()) {

@joshelser joshelser Sep 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about making this whole for-loop:

quotaManager.snapshotRegionSize().keySet()
  .stream()
  .filter(k -> k.getTable().equals(tableName))
  .count() > 0`

@shardul-cr7 shardul-cr7 Sep 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

if (entry.getKey().getTable().equals(tableName)) {
assertTrue(true);
return;
}
}
Assert.fail("Testcase failed, disable entry removed");
}
}