From 8f515fe99161fee347ee03b61e0d8353b93e0e4e Mon Sep 17 00:00:00 2001 From: Nick Dimiduk Date: Tue, 23 Jul 2024 15:49:19 +0200 Subject: [PATCH] HBASE-28716 Users of QuotaRetriever should pass an existing connection (#6065) Signed-off-by: Nick Dimiduk Signed-off-by: Pankaj Kumar Co-authored-by: Charles Connell --- .../hadoop/hbase/client/HBaseAdmin.java | 10 ++--- .../hadoop/hbase/quotas/QuotaRetriever.java | 31 +++++++++---- .../hbase/quotas/QuotaObserverChore.java | 3 +- .../quotas/SnapshotQuotaObserverChore.java | 4 +- .../resources/hbase-webapps/master/quotas.jsp | 3 +- .../quotas/SpaceQuotaHelperForTests.java | 18 ++------ .../quotas/TestMasterQuotasObserver.java | 26 +++++------ .../hadoop/hbase/quotas/TestQuotaAdmin.java | 43 +++++-------------- 8 files changed, 60 insertions(+), 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 index 68a30829e724..d8b9c9dd9a68 100644 --- 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 getQuota(QuotaFilter filter) throws IOException { List quotas = new ArrayList<>(); - try (QuotaRetriever retriever = QuotaRetriever.open(conf, filter)) { - Iterator 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 index 1dd5bf275bba..1d902ff9b718 100644 --- 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 { /** * 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 index 32bfcebcb9e4..a89db895cb4c 100644 --- 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 index cfdcb52db71c..c198f3d9d322 100644 --- 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 getSnapshotsToComputeSize() throws IOException { Set 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 index 780a8d4b3605..a52085b529f9 100644 --- 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 regionServerThrottles = new ArrayList<>(); List 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 index e8693077ed5c..e0c09315024e 100644 --- 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 index bc547b5796ac..e1684240bdc8 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 @@ -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 index a3b2929b9aa8..5b560129ecea 100644 --- 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(); } }