HBASE-28716 Users of QuotaRetriever should pass an existing connection (

apache#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Co-authored-by: Charles Connell <[email protected]>
ndimiduk · Jul 23, 2024 · 8f515fe · 8f515fe
1 parent 2d83a8a
commit 8f515fe
Show file tree

Hide file tree

Showing 8 changed files with 60 additions and 78 deletions.
diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
@@ -29,7 +29,6 @@
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.HashMap;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -3153,16 +3152,15 @@ protected Void rpcCall() throws Exception {
 
   @Override
   public QuotaRetriever getQuotaRetriever(final QuotaFilter filter) throws IOException {
-    return QuotaRetriever.open(conf, filter);
+    return new QuotaRetriever(connection, filter);
   }
 
   @Override
   public List<QuotaSettings> getQuota(QuotaFilter filter) throws IOException {
     List<QuotaSettings> quotas = new ArrayList<>();
-    try (QuotaRetriever retriever = QuotaRetriever.open(conf, filter)) {
-      Iterator<QuotaSettings> iterator = retriever.iterator();
-      while (iterator.hasNext()) {
-        quotas.add(iterator.next());
+    try (QuotaRetriever retriever = new QuotaRetriever(connection, filter)) {
+      for (QuotaSettings quotaSettings : retriever) {
+        quotas.add(quotaSettings);
       }
     }
     return quotas;

diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/QuotaRetriever.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/QuotaRetriever.java
@@ -55,19 +55,29 @@ public class QuotaRetriever implements Closeable, Iterable<QuotaSettings> {
   /**
    * Should QutoaRetriever manage the state of the connection, or leave it be.
    */
-  private boolean isManagedConnection = false;
+  private final boolean isManagedConnection;
 
-  QuotaRetriever() {
+  public QuotaRetriever(final Connection conn) throws IOException {
+    this(conn, (QuotaFilter) null);
   }
 
-  void init(final Configuration conf, final Scan scan) throws IOException {
+  public QuotaRetriever(final Connection conn, final QuotaFilter filter) throws IOException {
+    this(conn, QuotaTableUtil.makeScan(filter));
+  }
+
+  public QuotaRetriever(final Connection conn, final Scan scan) throws IOException {
+    isManagedConnection = false;
+    init(conn, scan);
+  }
+
+  QuotaRetriever(final Configuration conf, final Scan scan) throws IOException {
     // Set this before creating the connection and passing it down to make sure
     // it's cleaned up if we fail to construct the Scanner.
-    this.isManagedConnection = true;
+    isManagedConnection = true;
     init(ConnectionFactory.createConnection(conf), scan);
   }
 
-  void init(final Connection conn, final Scan scan) throws IOException {
+  private void init(final Connection conn, final Scan scan) throws IOException {
     this.connection = Objects.requireNonNull(conn);
     this.table = this.connection.getTable(QuotaTableUtil.QUOTA_TABLE_NAME);
     try {
@@ -159,7 +169,10 @@ public void remove() {
    * @param conf Configuration object to use.
    * @return the QuotaRetriever
    * @throws IOException if a remote or network exception occurs
+   * @deprecated Since 3.0.0, will be removed in 4.0.0. Use
+   *             {@link #QuotaRetriever(Configuration, Scan)} instead.
    */
+  @Deprecated
   public static QuotaRetriever open(final Configuration conf) throws IOException {
     return open(conf, null);
   }
@@ -170,12 +183,14 @@ public static QuotaRetriever open(final Configuration conf) throws IOException {
    * @param filter the QuotaFilter
    * @return the QuotaRetriever
    * @throws IOException if a remote or network exception occurs
+   * @deprecated Since 3.0.0, will be removed in 4.0.0. Use
+   *             {@link #QuotaRetriever(Configuration, Scan)} instead.
    */
+  @Deprecated
   public static QuotaRetriever open(final Configuration conf, final QuotaFilter filter)
     throws IOException {
     Scan scan = QuotaTableUtil.makeScan(filter);
-    QuotaRetriever scanner = new QuotaRetriever();
-    scanner.init(conf, scan);
-    return scanner;
+    return new QuotaRetriever(conf, scan);
   }
+
 }
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaObserverChore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaObserverChore.java
@@ -475,8 +475,7 @@ void pruneOldRegionReports() {
   TablesWithQuotas fetchAllTablesWithQuotasDefined() throws IOException {
     final Scan scan = QuotaTableUtil.makeScan(null);
     final TablesWithQuotas tablesWithQuotas = new TablesWithQuotas(conn, conf);
-    try (final QuotaRetriever scanner = new QuotaRetriever()) {
-      scanner.init(conn, scan);
+    try (final QuotaRetriever scanner = new QuotaRetriever(conn, scan)) {
       for (QuotaSettings quotaSettings : scanner) {
         // Only one of namespace and tablename should be 'null'
         final String namespace = quotaSettings.getNamespace();

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/SnapshotQuotaObserverChore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/SnapshotQuotaObserverChore.java
@@ -166,9 +166,9 @@ Multimap<TableName, String> getSnapshotsToComputeSize() throws IOException {
     Set<TableName> tablesToFetchSnapshotsFrom = new HashSet<>();
     QuotaFilter filter = new QuotaFilter();
     filter.addTypeFilter(QuotaType.SPACE);
-    try (Admin admin = conn.getAdmin()) {
+    try (Admin admin = conn.getAdmin(); QuotaRetriever qr = new QuotaRetriever(conn, filter)) {
       // Pull all of the tables that have quotas (direct, or from namespace)
-      for (QuotaSettings qs : QuotaRetriever.open(conf, filter)) {
+      for (QuotaSettings qs : qr) {
         if (qs.getQuotaType() == QuotaType.SPACE) {
           String ns = qs.getNamespace();
           TableName tn = qs.getTableName();

diff --git a/hbase-server/src/main/resources/hbase-webapps/master/quotas.jsp b/hbase-server/src/main/resources/hbase-webapps/master/quotas.jsp
@@ -30,7 +30,6 @@
 %>
 <%
   HMaster master = (HMaster) getServletContext().getAttribute(HMaster.MASTER);
-  Configuration conf = master.getConfiguration();
   pageContext.setAttribute("pageTitle", "HBase Master Quotas: " + master.getServerName());
   List<ThrottleSettings> regionServerThrottles = new ArrayList<>();
   List<ThrottleSettings> namespaceThrottles = new ArrayList<>();
@@ -39,7 +38,7 @@
   boolean exceedThrottleQuotaEnabled = false;
   if (quotaManager != null) {
     exceedThrottleQuotaEnabled = quotaManager.isExceedThrottleQuotaEnabled();
-    try (QuotaRetriever scanner = QuotaRetriever.open(conf, null)) {
+    try (QuotaRetriever scanner = new QuotaRetriever(master.getConnection())) {
       for (QuotaSettings quota : scanner) {
         if (quota instanceof ThrottleSettings) {
           ThrottleSettings throttle = (ThrottleSettings) quota;

diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/SpaceQuotaHelperForTests.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/SpaceQuotaHelperForTests.java
@@ -124,13 +124,8 @@ static void updateConfigForQuotas(Configuration conf) {
    * Returns the number of quotas defined in the HBase quota table.
    */
   long listNumDefinedQuotas(Connection conn) throws IOException {
-    QuotaRetriever scanner = QuotaRetriever.open(conn.getConfiguration());
-    try {
+    try (QuotaRetriever scanner = new QuotaRetriever(conn)) {
       return Iterables.size(scanner);
-    } finally {
-      if (scanner != null) {
-        scanner.close();
-      }
     }
   }
 
@@ -436,8 +431,7 @@ void removeAllQuotas(Connection conn) throws IOException {
       waitForQuotaTable(conn);
     } else {
       // Or, clean up any quotas from previous test runs.
-      QuotaRetriever scanner = QuotaRetriever.open(conn.getConfiguration());
-      try {
+      try (QuotaRetriever scanner = new QuotaRetriever(conn);) {
         for (QuotaSettings quotaSettings : scanner) {
           final String namespace = quotaSettings.getNamespace();
           final TableName tableName = quotaSettings.getTableName();
@@ -453,17 +447,13 @@ void removeAllQuotas(Connection conn) throws IOException {
             QuotaUtil.deleteUserQuota(conn, userName);
           }
         }
-      } finally {
-        if (scanner != null) {
-          scanner.close();
-        }
       }
     }
   }
 
   QuotaSettings getTableSpaceQuota(Connection conn, TableName tn) throws IOException {
-    try (QuotaRetriever scanner = QuotaRetriever.open(conn.getConfiguration(),
-      new QuotaFilter().setTableFilter(tn.getNameAsString()))) {
+    try (QuotaRetriever scanner =
+      new QuotaRetriever(conn, new QuotaFilter().setTableFilter(tn.getNameAsString()))) {
       for (QuotaSettings setting : scanner) {
         if (setting.getTableName().equals(tn) && setting.getQuotaType() == QuotaType.SPACE) {
           return setting;

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
@@ -325,25 +325,27 @@ public boolean namespaceExists(String ns) throws IOException {
   }
 
   public int getNumSpaceQuotas() throws Exception {
-    QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration());
-    int numSpaceQuotas = 0;
-    for (QuotaSettings quotaSettings : scanner) {
-      if (quotaSettings.getQuotaType() == QuotaType.SPACE) {
-        numSpaceQuotas++;
+    try (QuotaRetriever scanner = new QuotaRetriever(TEST_UTIL.getConnection())) {
+      int numSpaceQuotas = 0;
+      for (QuotaSettings quotaSettings : scanner) {
+        if (quotaSettings.getQuotaType() == QuotaType.SPACE) {
+          numSpaceQuotas++;
+        }
       }
+      return numSpaceQuotas;
     }
-    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++;
+    try (QuotaRetriever scanner = new QuotaRetriever(TEST_UTIL.getConnection())) {
+      int throttleQuotas = 0;
+      for (QuotaSettings quotaSettings : scanner) {
+        if (quotaSettings.getQuotaType() == QuotaType.THROTTLE) {
+          throttleQuotas++;
+        }
       }
+      return throttleQuotas;
     }
-    return throttleQuotas;
   }
 
   private void createTable(Admin admin, TableName tn) throws Exception {

diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaAdmin.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaAdmin.java
@@ -123,7 +123,7 @@ public void testThrottleType() throws Exception {
       QuotaSettingsFactory.throttleUser(userName, ThrottleType.WRITE_NUMBER, 12, TimeUnit.MINUTES));
     admin.setQuota(QuotaSettingsFactory.bypassGlobals(userName, true));
 
-    try (QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration())) {
+    try (QuotaRetriever scanner = new QuotaRetriever(TEST_UTIL.getConnection())) {
       int countThrottle = 0;
       int countGlobalBypass = 0;
       for (QuotaSettings settings : scanner) {
@@ -169,7 +169,7 @@ public void testSimpleScan() throws Exception {
       TimeUnit.MINUTES));
     admin.setQuota(QuotaSettingsFactory.bypassGlobals(userName, true));
 
-    try (QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration())) {
+    try (QuotaRetriever scanner = new QuotaRetriever(TEST_UTIL.getConnection())) {
       int countThrottle = 0;
       int countGlobalBypass = 0;
       for (QuotaSettings settings : scanner) {
@@ -345,11 +345,8 @@ public void testSetGetRemoveSpaceQuota() throws Exception {
     }
 
     // Verify we can retrieve it via the QuotaRetriever API
-    QuotaRetriever scanner = QuotaRetriever.open(admin.getConfiguration());
-    try {
+    try (QuotaRetriever scanner = new QuotaRetriever(admin.getConnection())) {
       assertSpaceQuota(sizeLimit, violationPolicy, Iterables.getOnlyElement(scanner));
-    } finally {
-      scanner.close();
     }
 
     // Now, remove the quota
@@ -367,11 +364,8 @@ public void testSetGetRemoveSpaceQuota() throws Exception {
     }
 
     // Verify that we can also not fetch it via the API
-    scanner = QuotaRetriever.open(admin.getConfiguration());
-    try {
+    try (QuotaRetriever scanner = new QuotaRetriever(admin.getConnection())) {
       assertNull("Did not expect to find a quota entry", scanner.next());
-    } finally {
-      scanner.close();
     }
   }
 
@@ -399,11 +393,8 @@ public void testSetModifyRemoveSpaceQuota() throws Exception {
     }
 
     // Verify we can retrieve it via the QuotaRetriever API
-    QuotaRetriever quotaScanner = QuotaRetriever.open(admin.getConfiguration());
-    try {
+    try (QuotaRetriever quotaScanner = new QuotaRetriever(admin.getConnection())) {
       assertSpaceQuota(originalSizeLimit, violationPolicy, Iterables.getOnlyElement(quotaScanner));
-    } finally {
-      quotaScanner.close();
     }
 
     // Setting a new size and policy should be reflected
@@ -427,11 +418,8 @@ public void testSetModifyRemoveSpaceQuota() throws Exception {
     }
 
     // Verify we can retrieve the new quota via the QuotaRetriever API
-    quotaScanner = QuotaRetriever.open(admin.getConfiguration());
-    try {
+    try (QuotaRetriever quotaScanner = new QuotaRetriever(admin.getConnection())) {
       assertSpaceQuota(newSizeLimit, newViolationPolicy, Iterables.getOnlyElement(quotaScanner));
-    } finally {
-      quotaScanner.close();
     }
 
     // Now, remove the quota
@@ -449,11 +437,8 @@ public void testSetModifyRemoveSpaceQuota() throws Exception {
     }
 
     // Verify that we can also not fetch it via the API
-    quotaScanner = QuotaRetriever.open(admin.getConfiguration());
-    try {
+    try (QuotaRetriever quotaScanner = new QuotaRetriever(admin.getConnection())) {
       assertNull("Did not expect to find a quota entry", quotaScanner.next());
-    } finally {
-      quotaScanner.close();
     }
   }
 
@@ -549,8 +534,7 @@ public void testSetAndRemoveRegionServerQuota() throws Exception {
     admin.setQuota(QuotaSettingsFactory.throttleRegionServer(regionServer, ThrottleType.READ_NUMBER,
       30, TimeUnit.SECONDS));
     int count = 0;
-    QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration(), rsFilter);
-    try {
+    try (QuotaRetriever scanner = new QuotaRetriever(TEST_UTIL.getConnection(), rsFilter)) {
       for (QuotaSettings settings : scanner) {
         assertTrue(settings.getQuotaType() == QuotaType.THROTTLE);
         ThrottleSettings throttleSettings = (ThrottleSettings) settings;
@@ -564,8 +548,6 @@ public void testSetAndRemoveRegionServerQuota() throws Exception {
           assertEquals(TimeUnit.SECONDS, throttleSettings.getTimeUnit());
         }
       }
-    } finally {
-      scanner.close();
     }
     assertEquals(2, count);
 
@@ -733,14 +715,14 @@ private void verifyRecordNotPresentInQuotaTable() throws Exception {
   private void verifyFetchableViaAPI(Admin admin, ThrottleType type, long limit, TimeUnit tu)
     throws Exception {
     // Verify we can retrieve the new quota via the QuotaRetriever API
-    try (QuotaRetriever quotaScanner = QuotaRetriever.open(admin.getConfiguration())) {
+    try (QuotaRetriever quotaScanner = new QuotaRetriever(admin.getConnection())) {
       assertRPCQuota(type, limit, tu, Iterables.getOnlyElement(quotaScanner));
     }
   }
 
   private void verifyNotFetchableViaAPI(Admin admin) throws Exception {
     // Verify that we can also not fetch it via the API
-    try (QuotaRetriever quotaScanner = QuotaRetriever.open(admin.getConfiguration())) {
+    try (QuotaRetriever quotaScanner = new QuotaRetriever(admin.getConnection())) {
       assertNull("Did not expect to find a quota entry", quotaScanner.next());
     }
   }
@@ -830,16 +812,13 @@ private void assertSpaceQuota(long sizeLimit, SpaceViolationPolicy violationPoli
   }
 
   private int countResults(final QuotaFilter filter) throws Exception {
-    QuotaRetriever scanner = QuotaRetriever.open(TEST_UTIL.getConfiguration(), filter);
-    try {
+    try (QuotaRetriever scanner = new QuotaRetriever(TEST_UTIL.getConnection(), filter)) {
       int count = 0;
       for (QuotaSettings settings : scanner) {
         LOG.debug(Objects.toString(settings));
         count++;
       }
       return count;
-    } finally {
-      scanner.close();
     }
   }