HBASE-20813 Removed RPC quotas when the associated table/Namespace is dropped off

Signed-off-by: Josh Elser <elserj@apache.org>

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

@@ -158,7 +158,7 @@ import org.apache.hadoop.hbase.procedure2.RemoteProcedureDispatcher.RemoteProced
 import org.apache.hadoop.hbase.procedure2.RemoteProcedureException;
 import org.apache.hadoop.hbase.procedure2.store.wal.WALProcedureStore;
 import org.apache.hadoop.hbase.quotas.MasterQuotaManager;
-import org.apache.hadoop.hbase.quotas.MasterSpaceQuotaObserver;
+import org.apache.hadoop.hbase.quotas.MasterQuotasObserver;
 import org.apache.hadoop.hbase.quotas.QuotaObserverChore;
 import org.apache.hadoop.hbase.quotas.QuotaUtil;
 import org.apache.hadoop.hbase.quotas.SnapshotQuotaObserverChore;
@@ -910,10 +910,10 @@ public class HMaster extends HRegionServer implements MasterServices {
         new ReplicationPeerConfigUpgrader(zooKeeper, conf);
     tableCFsUpdater.copyTableCFs();
 
-    // Add the Observer to delete space quotas on table deletion before starting all CPs by
+    // Add the Observer to delete quotas on table deletion before starting all CPs by
     // default with quota support, avoiding if user specifically asks to not load this Observer.
     if (QuotaUtil.isQuotaEnabled(conf)) {
-      updateConfigurationForSpaceQuotaObserver(conf);
+      updateConfigurationForQuotasObserver(conf);
     }
     // initialize master side coprocessors before we start handling requests
     status.setStatus("Initializing master coprocessors");
@@ -1069,15 +1069,15 @@ public class HMaster extends HRegionServer implements MasterServices {
   }
 
   /**
-   * Adds the {@code MasterSpaceQuotaObserver} to the list of configured Master observers to
+   * Adds the {@code MasterQuotasObserver} to the list of configured Master observers to
-   * automatically remove space quotas for a table when that table is deleted.
+   * automatically remove quotas for a table when that table is deleted.
    */
   @VisibleForTesting
-  public void updateConfigurationForSpaceQuotaObserver(Configuration conf) {
+  public void updateConfigurationForQuotasObserver(Configuration conf) {
     // We're configured to not delete quotas on table deletion, so we don't need to add the obs.
     if (!conf.getBoolean(
-          MasterSpaceQuotaObserver.REMOVE_QUOTA_ON_TABLE_DELETE,
+          MasterQuotasObserver.REMOVE_QUOTA_ON_TABLE_DELETE,
-          MasterSpaceQuotaObserver.REMOVE_QUOTA_ON_TABLE_DELETE_DEFAULT)) {
+          MasterQuotasObserver.REMOVE_QUOTA_ON_TABLE_DELETE_DEFAULT)) {
       return;
     }
     String[] masterCoprocs = conf.getStrings(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
@@ -1086,7 +1086,7 @@ public class HMaster extends HRegionServer implements MasterServices {
     if (length > 0) {
       System.arraycopy(masterCoprocs, 0, updatedCoprocs, 0, masterCoprocs.length);
     }
-    updatedCoprocs[length] = MasterSpaceQuotaObserver.class.getName();
+    updatedCoprocs[length] = MasterQuotasObserver.class.getName();
     conf.setStrings(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, updatedCoprocs);
   }
 

hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/MasterQuotasObserver.java

@@ -22,21 +22,21 @@ import java.util.Optional;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.CoprocessorEnvironment;
 import org.apache.hadoop.hbase.TableName;
-import org.apache.yetus.audience.InterfaceAudience;
 import org.apache.hadoop.hbase.client.Admin;
 import org.apache.hadoop.hbase.client.Connection;
 import org.apache.hadoop.hbase.coprocessor.MasterCoprocessor;
 import org.apache.hadoop.hbase.coprocessor.MasterCoprocessorEnvironment;
 import org.apache.hadoop.hbase.coprocessor.MasterObserver;
 import org.apache.hadoop.hbase.coprocessor.ObserverContext;
+import org.apache.yetus.audience.InterfaceAudience;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.QuotaProtos.Quotas;
 
 /**
- * An observer to automatically delete space quotas when a table/namespace
+ * An observer to automatically delete quotas when a table/namespace
- * are deleted.
+ * is deleted.
  */
 @InterfaceAudience.Private
-public class MasterSpaceQuotaObserver implements MasterCoprocessor, MasterObserver {
+public class MasterQuotasObserver implements MasterCoprocessor, MasterObserver {
   public static final String REMOVE_QUOTA_ON_TABLE_DELETE = "hbase.quota.remove.on.table.delete";
   public static final boolean REMOVE_QUOTA_ON_TABLE_DELETE_DEFAULT = true;
 
@@ -65,10 +65,18 @@ public class MasterSpaceQuotaObserver implements MasterCoprocessor, MasterObserv
     }
     final Connection conn = ctx.getEnvironment().getConnection();
     Quotas quotas = QuotaUtil.getTableQuota(conn, tableName);
-    if (quotas != null && quotas.hasSpace()) {
+    if (quotas != null){
-      QuotaSettings settings = QuotaSettingsFactory.removeTableSpaceLimit(tableName);
+      if (quotas.hasSpace()){
-      try (Admin admin = conn.getAdmin()) {
+        QuotaSettings settings = QuotaSettingsFactory.removeTableSpaceLimit(tableName);
-        admin.setQuota(settings);
+        try (Admin admin = conn.getAdmin()) {
+          admin.setQuota(settings);
+        }
+      }
+      if (quotas.hasThrottle()){
+        QuotaSettings settings = QuotaSettingsFactory.unthrottleTable(tableName);
+        try (Admin admin = conn.getAdmin()) {
+          admin.setQuota(settings);
+        }
       }
     }
   }
@@ -82,11 +90,19 @@ public class MasterSpaceQuotaObserver implements MasterCoprocessor, MasterObserv
     }
     final Connection conn = ctx.getEnvironment().getConnection();
     Quotas quotas = QuotaUtil.getNamespaceQuota(conn, namespace);
-    if (quotas != null && quotas.hasSpace()) {
+    if (quotas != null) {
-      QuotaSettings settings = QuotaSettingsFactory.removeNamespaceSpaceLimit(namespace);
+      if (quotas.hasSpace()) {
-      try (Admin admin = conn.getAdmin()) {
+        QuotaSettings settings = QuotaSettingsFactory.removeNamespaceSpaceLimit(namespace);
-        admin.setQuota(settings);
+        try (Admin admin = conn.getAdmin()) {
+          admin.setQuota(settings);
+        }
+      }
+      if (quotas.hasThrottle()) {
+        QuotaSettings settings = QuotaSettingsFactory.unthrottleNamespace(namespace);
+        try (Admin admin = conn.getAdmin()) {
+          admin.setQuota(settings);
+        }
       }
     }
   }
-}
+}

hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestMasterQuotasObserver.java

@@ -22,6 +22,7 @@ import static org.junit.Assert.assertTrue;
 
 import java.io.IOException;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
@@ -32,7 +33,6 @@ import org.apache.hadoop.hbase.NamespaceDescriptor;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.client.Admin;
 import org.apache.hadoop.hbase.client.Connection;
-import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
 import org.apache.hadoop.hbase.master.HMaster;
 import org.apache.hadoop.hbase.master.MasterCoprocessorHost;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
@@ -46,14 +46,14 @@ import org.junit.experimental.categories.Category;
 import org.junit.rules.TestName;
 
 /**
- * Test class for {@link MasterSpaceQuotaObserver}.
+ * Test class for {@link MasterQuotasObserver}.
  */
 @Category(MediumTests.class)
-public class TestMasterSpaceQuotaObserver {
+public class TestMasterQuotasObserver {
 
   @ClassRule
   public static final HBaseClassTestRule CLASS_RULE =
-      HBaseClassTestRule.forClass(TestMasterSpaceQuotaObserver.class);
+      HBaseClassTestRule.forClass(TestMasterQuotasObserver.class);
 
   private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();
   private static SpaceQuotaHelperForTests helper;
@@ -90,36 +90,55 @@ public class TestMasterSpaceQuotaObserver {
   }
 
   @Test
-  public void testTableQuotaRemoved() throws Exception {
+  public void testTableSpaceQuotaRemoved() throws Exception {
     final Connection conn = TEST_UTIL.getConnection();
     final Admin admin = conn.getAdmin();
     final TableName tn = TableName.valueOf(testName.getMethodName());
     // Drop the table if it somehow exists
     if (admin.tableExists(tn)) {
-      admin.disableTable(tn);
+      dropTable(admin, tn);
-      admin.deleteTable(tn);
     }
-
+    createTable(admin, tn);
-    // Create a table
-    HTableDescriptor tableDesc = new HTableDescriptor(tn);
-    tableDesc.addFamily(new HColumnDescriptor("F1"));
-    admin.createTable(tableDesc);
     assertEquals(0, getNumSpaceQuotas());
 
-    // Set a quota
+    // Set space quota
     QuotaSettings settings = QuotaSettingsFactory.limitTableSpace(
         tn, 1024L, SpaceViolationPolicy.NO_INSERTS);
     admin.setQuota(settings);
     assertEquals(1, getNumSpaceQuotas());
 
-    // Delete the table and observe the quota being automatically deleted as well
+    // Drop the table and observe the Space quota being automatically deleted as well
-    admin.disableTable(tn);
+    dropTable(admin, tn);
-    admin.deleteTable(tn);
     assertEquals(0, getNumSpaceQuotas());
   }
 
   @Test
-  public void testNamespaceQuotaRemoved() throws Exception {
+  public void testTableRPCQuotaRemoved() throws Exception {
+    final Connection conn = TEST_UTIL.getConnection();
+    final Admin admin = conn.getAdmin();
+    final TableName tn = TableName.valueOf(testName.getMethodName());
+    // Drop the table if it somehow exists
+    if (admin.tableExists(tn)) {
+      dropTable(admin, tn);
+    }
+
+    createTable(admin, tn);
+    assertEquals(0, getThrottleQuotas());
+
+    // Set RPC quota
+    QuotaSettings settings =
+        QuotaSettingsFactory.throttleTable(tn, ThrottleType.REQUEST_SIZE, 2L, TimeUnit.HOURS);
+    admin.setQuota(settings);
+
+    assertEquals(1, getThrottleQuotas());
+
+    // Delete the table and observe the RPC quota being automatically deleted as well
+    dropTable(admin, tn);
+    assertEquals(0, getThrottleQuotas());
+  }
+
+  @Test
+  public void testNamespaceSpaceQuotaRemoved() throws Exception {
     final Connection conn = TEST_UTIL.getConnection();
     final Admin admin = conn.getAdmin();
     final String ns = testName.getMethodName();
@@ -139,19 +158,45 @@ public class TestMasterSpaceQuotaObserver {
     admin.setQuota(settings);
     assertEquals(1, getNumSpaceQuotas());
 
-    // Delete the table and observe the quota being automatically deleted as well
+    // Delete the namespace and observe the quota being automatically deleted as well
     admin.deleteNamespace(ns);
     assertEquals(0, getNumSpaceQuotas());
   }
 
+  @Test
+  public void testNamespaceRPCQuotaRemoved() throws Exception {
+    final Connection conn = TEST_UTIL.getConnection();
+    final Admin admin = conn.getAdmin();
+    final String ns = testName.getMethodName();
+    // Drop the ns if it somehow exists
+    if (namespaceExists(ns)) {
+      admin.deleteNamespace(ns);
+    }
+
+    // Create the ns
+    NamespaceDescriptor desc = NamespaceDescriptor.create(ns).build();
+    admin.createNamespace(desc);
+    assertEquals(0, getThrottleQuotas());
+
+    // Set a quota
+    QuotaSettings settings =
+        QuotaSettingsFactory.throttleNamespace(ns, ThrottleType.REQUEST_SIZE, 2L, TimeUnit.HOURS);
+    admin.setQuota(settings);
+    assertEquals(1, getThrottleQuotas());
+
+    // Delete the namespace and observe the quota being automatically deleted as well
+    admin.deleteNamespace(ns);
+    assertEquals(0, getThrottleQuotas());
+  }
+
   @Test
   public void testObserverAddedByDefault() throws Exception {
     final HMaster master = TEST_UTIL.getHBaseCluster().getMaster();
     final MasterCoprocessorHost cpHost = master.getMasterCoprocessorHost();
     Set<String> coprocessorNames = cpHost.getCoprocessors();
     assertTrue(
-        "Did not find MasterSpaceQuotaObserver in list of CPs: " + coprocessorNames,
+        "Did not find MasterQuotasObserver in list of CPs: " + coprocessorNames,
-        coprocessorNames.contains(MasterSpaceQuotaObserver.class.getSimpleName()));
+        coprocessorNames.contains(MasterQuotasObserver.class.getSimpleName()));
   }
 
   public boolean namespaceExists(String ns) throws IOException {
@@ -174,4 +219,27 @@ public class TestMasterSpaceQuotaObserver {
     }
     return numSpaceQuotas;
   }
+
+  public int getThrottleQuotas() throws Exception {
+    QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration());
+    int throttleQuotas = 0;
+    for (QuotaSettings quotaSettings : scanner) {
+      if (quotaSettings.getQuotaType() == QuotaType.THROTTLE) {
+        throttleQuotas++;
+      }
+    }
+    return throttleQuotas;
+  }
+
+  private void createTable(Admin admin, TableName tn) throws Exception {
+    // Create a table
+    HTableDescriptor tableDesc = new HTableDescriptor(tn);
+    tableDesc.addFamily(new HColumnDescriptor("F1"));
+    admin.createTable(tableDesc);
+  }
+
+  private void dropTable(Admin admin, TableName tn) throws  Exception {
+    admin.disableTable(tn);
+    admin.deleteTable(tn);
+  }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestMasterQuotasObserverWithMocks.java

@@ -18,7 +18,7 @@
 package org.apache.hadoop.hbase.quotas;
 
 import static org.apache.hadoop.hbase.coprocessor.CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY;
-import static org.apache.hadoop.hbase.quotas.MasterSpaceQuotaObserver.REMOVE_QUOTA_ON_TABLE_DELETE;
+import static org.apache.hadoop.hbase.quotas.MasterQuotasObserver.REMOVE_QUOTA_ON_TABLE_DELETE;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -40,14 +40,14 @@ import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
 /**
- * Test class for MasterSpaceQuotaObserver that does not require a cluster.
+ * Test class for MasterQuotasObserver that does not require a cluster.
  */
 @Category(SmallTests.class)
-public class TestMasterSpaceQuotaObserverWithMocks {
+public class TestMasterQuotasObserverWithMocks {
 
   @ClassRule
   public static final HBaseClassTestRule CLASS_RULE =
-      HBaseClassTestRule.forClass(TestMasterSpaceQuotaObserverWithMocks.class);
+      HBaseClassTestRule.forClass(TestMasterQuotasObserverWithMocks.class);
 
   private HMaster master;
   private Configuration conf;
@@ -56,20 +56,20 @@ public class TestMasterSpaceQuotaObserverWithMocks {
   public void setup() {
     conf = HBaseConfiguration.create();
     master = mock(HMaster.class);
-    doCallRealMethod().when(master).updateConfigurationForSpaceQuotaObserver(
+    doCallRealMethod().when(master).updateConfigurationForQuotasObserver(
         any());
   }
 
   @Test
   public void testAddDefaultObserver() {
-    master.updateConfigurationForSpaceQuotaObserver(conf);
+    master.updateConfigurationForQuotasObserver(conf);
-    assertEquals(MasterSpaceQuotaObserver.class.getName(), conf.get(MASTER_COPROCESSOR_CONF_KEY));
+    assertEquals(MasterQuotasObserver.class.getName(), conf.get(MASTER_COPROCESSOR_CONF_KEY));
   }
 
   @Test
   public void testDoNotAddDefaultObserver() {
     conf.setBoolean(REMOVE_QUOTA_ON_TABLE_DELETE, false);
-    master.updateConfigurationForSpaceQuotaObserver(conf);
+    master.updateConfigurationForQuotasObserver(conf);
     // Configuration#getStrings returns null when unset
     assertNull(conf.getStrings(MASTER_COPROCESSOR_CONF_KEY));
   }
@@ -77,7 +77,7 @@ public class TestMasterSpaceQuotaObserverWithMocks {
   @Test
   public void testAppendsObserver() {
     conf.set(MASTER_COPROCESSOR_CONF_KEY, AccessController.class.getName());
-    master.updateConfigurationForSpaceQuotaObserver(conf);
+    master.updateConfigurationForQuotasObserver(conf);
     Set<String> coprocs = new HashSet<>(conf.getStringCollection(MASTER_COPROCESSOR_CONF_KEY));
     assertEquals(2, coprocs.size());
     assertTrue(
@@ -85,6 +85,6 @@ public class TestMasterSpaceQuotaObserverWithMocks {
         coprocs.contains(AccessController.class.getName()));
     assertTrue(
         "Observed coprocessors were: " + coprocs,
-        coprocs.contains(MasterSpaceQuotaObserver.class.getName()));
+        coprocs.contains(MasterQuotasObserver.class.getName()));
   }
-}
+}