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 <elserj@apache.org>

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) {
+    validateSizeLimit(sizeLimit);
-      throw new IllegalArgumentException("Size limit must be a non-negative value.");
-    }
     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) {
+    validateSizeLimit(sizeLimit);
-      throw new IllegalArgumentException("Size limit must be a non-negative value.");
-    }
     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.");
+    }
+  }
 }

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 &&
+    if (throttleBuilder == null && removeSpaceBuilder && bypassGlobals == null) {
-        (spaceBuilder == null || (spaceBuilder.hasRemove() && spaceBuilder.getRemove()))
-        && 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();
-  }
 }

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());
   }