From 93d4b95b3fe871bbbfe9a62cdaa72d1c2d15efbe Mon Sep 17 00:00:00 2001 From: Sakthi Date: Fri, 11 Jan 2019 13:05:38 -0800 Subject: [PATCH] HBASE-21225: Having RPC & Space quota on a table/Namespace doesn't allow space quota to be removed using 'NONE' Signed-off-by: Josh Elser --- .../hbase/quotas/SpaceLimitSettings.java | 15 +++-- .../hbase/quotas/GlobalQuotaSettingsImpl.java | 23 +++---- .../quotas/TestMasterQuotasObserver.java | 61 +++++++++++++++++-- 3 files changed, 71 insertions(+), 28 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/SpaceLimitSettings.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/SpaceLimitSettings.java index 8b31e94a4a3..69baf62206a 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/SpaceLimitSettings.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/SpaceLimitSettings.java @@ -38,9 +38,7 @@ class SpaceLimitSettings extends QuotaSettings { SpaceLimitSettings(TableName tableName, long sizeLimit, SpaceViolationPolicy violationPolicy) { super(null, Objects.requireNonNull(tableName), null); - if (sizeLimit < 0L) { - throw new IllegalArgumentException("Size limit must be a non-negative value."); - } + validateSizeLimit(sizeLimit); proto = buildProtoAddQuota(sizeLimit, Objects.requireNonNull(violationPolicy)); } @@ -54,9 +52,7 @@ class SpaceLimitSettings extends QuotaSettings { SpaceLimitSettings(String namespace, long sizeLimit, SpaceViolationPolicy violationPolicy) { super(null, null, Objects.requireNonNull(namespace)); - if (sizeLimit < 0L) { - throw new IllegalArgumentException("Size limit must be a non-negative value."); - } + validateSizeLimit(sizeLimit); proto = buildProtoAddQuota(sizeLimit, Objects.requireNonNull(violationPolicy)); } @@ -240,4 +236,11 @@ class SpaceLimitSettings extends QuotaSettings { } return this; } + + // Helper function to validate sizeLimit + private void validateSizeLimit(long sizeLimit) { + if (sizeLimit < 0L) { + throw new IllegalArgumentException("Size limit must be a non-negative value."); + } + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/GlobalQuotaSettingsImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/GlobalQuotaSettingsImpl.java index e47e4ffbd12..78f52c48e90 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/GlobalQuotaSettingsImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/GlobalQuotaSettingsImpl.java @@ -121,6 +121,7 @@ public class GlobalQuotaSettingsImpl extends GlobalQuotaSettings { if (other instanceof ThrottleSettings) { ThrottleSettings otherThrottle = (ThrottleSettings) other; if (!otherThrottle.proto.hasType() || !otherThrottle.proto.hasTimedQuota()) { + // It means it's a remove request // To prevent the "empty" row in QuotaTableUtil.QUOTA_TABLE_NAME throttleBuilder = null; } else { @@ -186,6 +187,7 @@ public class GlobalQuotaSettingsImpl extends GlobalQuotaSettings { } if (quotaToMerge.getRemove()) { + // It means it's a remove request // Update the builder to propagate the removal spaceBuilder.setRemove(true).clearSoftLimit().clearViolationPolicy(); } else { @@ -195,21 +197,22 @@ public class GlobalQuotaSettingsImpl extends GlobalQuotaSettings { } } + boolean removeSpaceBuilder = + (spaceBuilder == null) || (spaceBuilder.hasRemove() && spaceBuilder.getRemove()); + Boolean bypassGlobals = this.bypassGlobals; if (other instanceof QuotaGlobalsSettingsBypass) { bypassGlobals = ((QuotaGlobalsSettingsBypass) other).getBypass(); } - if (throttleBuilder == null && - (spaceBuilder == null || (spaceBuilder.hasRemove() && spaceBuilder.getRemove())) - && bypassGlobals == null) { + if (throttleBuilder == null && removeSpaceBuilder && bypassGlobals == null) { return null; } return new GlobalQuotaSettingsImpl( getUserName(), getTableName(), getNamespace(), (throttleBuilder == null ? null : throttleBuilder.build()), bypassGlobals, - (spaceBuilder == null ? null : spaceBuilder.build())); + (removeSpaceBuilder ? null : spaceBuilder.build())); } private void validateTimedQuota(final TimedQuota timedQuota) throws IOException { @@ -315,16 +318,4 @@ public class GlobalQuotaSettingsImpl extends GlobalQuotaSettings { } return quotas; } - - private void clearThrottleBuilder(QuotaProtos.Throttle.Builder builder) { - builder.clearReadNum(); - builder.clearReadSize(); - builder.clearReqNum(); - builder.clearReqSize(); - builder.clearWriteNum(); - builder.clearWriteSize(); - builder.clearReadCapacityUnit(); - builder.clearReadCapacityUnit(); - builder.clearWriteCapacityUnit(); - } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestMasterQuotasObserver.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestMasterQuotasObserver.java index 92e1d9dd4fa..155e2ed2bf5 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestMasterQuotasObserver.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestMasterQuotasObserver.java @@ -146,16 +146,13 @@ public class TestMasterQuotasObserver { if (admin.tableExists(tn)) { dropTable(admin, tn); } - createTable(admin, tn); assertEquals(0, getNumSpaceQuotas()); assertEquals(0, getThrottleQuotas()); - // Set Both quotas QuotaSettings settings = QuotaSettingsFactory.limitTableSpace(tn, 1024L, SpaceViolationPolicy.NO_INSERTS); admin.setQuota(settings); - settings = QuotaSettingsFactory.throttleTable(tn, ThrottleType.REQUEST_SIZE, 2L, TimeUnit.HOURS); admin.setQuota(settings); @@ -163,8 +160,34 @@ public class TestMasterQuotasObserver { assertEquals(1, getNumSpaceQuotas()); assertEquals(1, getThrottleQuotas()); - // Delete the table and observe the quotas being automatically deleted as well + // Remove Space quota + settings = QuotaSettingsFactory.removeTableSpaceLimit(tn); + admin.setQuota(settings); + assertEquals(0, getNumSpaceQuotas()); + assertEquals(1, getThrottleQuotas()); + + // Set back the space quota + settings = QuotaSettingsFactory.limitTableSpace(tn, 1024L, SpaceViolationPolicy.NO_INSERTS); + admin.setQuota(settings); + assertEquals(1, getNumSpaceQuotas()); + assertEquals(1, getThrottleQuotas()); + + // Remove the throttle quota + settings = QuotaSettingsFactory.unthrottleTable(tn); + admin.setQuota(settings); + assertEquals(1, getNumSpaceQuotas()); + assertEquals(0, getThrottleQuotas()); + + // Set back the throttle quota + settings = + QuotaSettingsFactory.throttleTable(tn, ThrottleType.REQUEST_SIZE, 2L, TimeUnit.HOURS); + admin.setQuota(settings); + assertEquals(1, getNumSpaceQuotas()); + assertEquals(1, getThrottleQuotas()); + + // Drop the table and check that both the quotas have been dropped as well dropTable(admin, tn); + assertEquals(0, getNumSpaceQuotas()); assertEquals(0, getThrottleQuotas()); } @@ -225,7 +248,6 @@ public class TestMasterQuotasObserver { public void testNamespaceSpaceAndRPCQuotaRemoved() throws Exception { final Connection conn = TEST_UTIL.getConnection(); final Admin admin = conn.getAdmin(); - final TableName tn = TableName.valueOf(testName.getMethodName()); final String ns = testName.getMethodName(); // Drop the ns if it somehow exists if (namespaceExists(ns)) { @@ -235,6 +257,7 @@ public class TestMasterQuotasObserver { // Create the ns NamespaceDescriptor desc = NamespaceDescriptor.create(ns).build(); admin.createNamespace(desc); + assertEquals(0, getNumSpaceQuotas()); assertEquals(0, getThrottleQuotas()); @@ -250,8 +273,34 @@ public class TestMasterQuotasObserver { assertEquals(1, getNumSpaceQuotas()); assertEquals(1, getThrottleQuotas()); - // Delete the namespace and observe the quotas being automatically deleted as well + // Remove Space quota + settings = QuotaSettingsFactory.removeNamespaceSpaceLimit(ns); + admin.setQuota(settings); + assertEquals(0, getNumSpaceQuotas()); + assertEquals(1, getThrottleQuotas()); + + // Set back the space quota + settings = QuotaSettingsFactory.limitNamespaceSpace(ns, 1024L, SpaceViolationPolicy.NO_INSERTS); + admin.setQuota(settings); + assertEquals(1, getNumSpaceQuotas()); + assertEquals(1, getThrottleQuotas()); + + // Remove the throttle quota + settings = QuotaSettingsFactory.unthrottleNamespace(ns); + admin.setQuota(settings); + assertEquals(1, getNumSpaceQuotas()); + assertEquals(0, getThrottleQuotas()); + + // Set back the throttle quota + settings = + QuotaSettingsFactory.throttleNamespace(ns, ThrottleType.REQUEST_SIZE, 2L, TimeUnit.HOURS); + admin.setQuota(settings); + assertEquals(1, getNumSpaceQuotas()); + assertEquals(1, getThrottleQuotas()); + + // Delete the namespace and check that both the quotas have been dropped as well admin.deleteNamespace(ns); + assertEquals(0, getNumSpaceQuotas()); assertEquals(0, getThrottleQuotas()); }