HBASE-27486 Fix per-table MetricsTableLatencies/QueryMeter memory leak

hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsTableLatencies.java

@@ -142,4 +142,14 @@ public interface MetricsTableLatencies {
    */
   void updateCheckAndMutate(String nameAsString, long time);
 
+  /**
+   * Remove all latency histograms of the given table. Should be called when the table is no
+   * longer online on this RegionServer (e.g. dropped, moved out, disabled) to avoid unbounded
+   * growth of histogramsByTable and the associated metrics registry (see HBASE-27486 /
+   * HBASE-27681).
+   *
+   * @param tableName The table whose latency histograms should be removed.
+   */
+  void deleteTable(String tableName);
+
 }

hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsTableQueryMeter.java

@@ -54,4 +54,13 @@ public interface MetricsTableQueryMeter {
    * @param tableName The table the metric is for
    */
   void updateTableWriteQueryMeter(TableName tableName);
+
+  /**
+   * Remove the read/write query meters of the given table. Should be called when the table is no
+   * longer online on this RegionServer to avoid unbounded growth of metersByTable and the
+   * associated metric registry (see HBASE-27486 / HBASE-27681).
+   *
+   * @param tableName The table whose meters should be removed.
+   */
+  void deleteTable(TableName tableName);
 }

hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsTableLatenciesImpl.java

@@ -17,7 +17,7 @@
  */
 package org.apache.hadoop.hbase.regionserver;
 
-import java.util.HashMap;
+import java.util.concurrent.ConcurrentHashMap;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.metrics.BaseSourceImpl;
 import org.apache.hadoop.metrics2.MetricHistogram;
@@ -33,7 +33,8 @@
 @InterfaceAudience.Private
 public class MetricsTableLatenciesImpl extends BaseSourceImpl implements MetricsTableLatencies {
 
-  private final HashMap<TableName, TableHistograms> histogramsByTable = new HashMap<>();
+  private final ConcurrentHashMap<TableName, TableHistograms> histogramsByTable =
+      new ConcurrentHashMap<>();
 
   public static class TableHistograms {
     final MetricHistogram getTimeHisto;
@@ -124,14 +125,48 @@ public static String qualifyMetricsName(TableName tableName, String metric) {
   }
 
   public TableHistograms getOrCreateTableHistogram(String tableName) {
-    // TODO Java8's ConcurrentHashMap#computeIfAbsent would be stellar instead
     final TableName tn = TableName.valueOf(tableName);
-    TableHistograms latency = histogramsByTable.get(tn);
-    if (latency == null) {
-      latency = new TableHistograms(getMetricsRegistry(), tn);
-      histogramsByTable.put(tn, latency);
+    return histogramsByTable.computeIfAbsent(tn,
+        t -> new TableHistograms(getMetricsRegistry(), t));
+  }
+
+  /**
+   * Remove all latency histograms of the given table from both {@link #histogramsByTable} and the
+   * underlying {@link DynamicMetricsRegistry}. This should be called when the table is no longer
+   * online on this RegionServer to avoid unbounded growth (see HBASE-27486 / HBASE-27681).
+   *
+   * <p>Note: this only removes the histograms registered by {@link TableHistograms}. Other
+   * per-table metrics registered by different components (e.g.
+   * {@link MetricsTableQueryMeterImpl}) are cleaned up separately.</p>
+   *
+   * <p>Implementation detail: each {@code MutableHistogram} is registered in the underlying
+   * {@link DynamicMetricsRegistry#metricsMap} under its base name (e.g.
+   * {@code Namespace_default_table_foo_metric_getTime}); the {@code _num_ops / _min / _max /
+   * _mean / *_percentile} suffixes are produced dynamically at snapshot time and are NOT stored
+   * as separate map entries. Therefore we must call {@link DynamicMetricsRegistry#removeMetric}
+   * on the base name to actually stop them from showing up in JMX; calling
+   * {@code removeHistogramMetrics} alone is a no-op for this scenario.</p>
+   */
+  @Override
+  public void deleteTable(String tableName) {
+    final TableName tn = TableName.valueOf(tableName);
+    TableHistograms removed = histogramsByTable.remove(tn);
+    if (removed == null) {
+      return;
     }
-    return latency;
+    DynamicMetricsRegistry reg = getMetricsRegistry();
+    reg.removeMetric(qualifyMetricsName(tn, GET_TIME));
+    reg.removeMetric(qualifyMetricsName(tn, INCREMENT_TIME));
+    reg.removeMetric(qualifyMetricsName(tn, APPEND_TIME));
+    reg.removeMetric(qualifyMetricsName(tn, PUT_TIME));
+    reg.removeMetric(qualifyMetricsName(tn, PUT_BATCH_TIME));
+    reg.removeMetric(qualifyMetricsName(tn, DELETE_TIME));
+    reg.removeMetric(qualifyMetricsName(tn, DELETE_BATCH_TIME));
+    reg.removeMetric(qualifyMetricsName(tn, SCAN_TIME));
+    reg.removeMetric(qualifyMetricsName(tn, SCAN_SIZE));
+    reg.removeMetric(qualifyMetricsName(tn, CHECK_AND_DELETE_TIME));
+    reg.removeMetric(qualifyMetricsName(tn, CHECK_AND_PUT_TIME));
+    reg.removeMetric(qualifyMetricsName(tn, CHECK_AND_MUTATE_TIME));
   }
 
   public MetricsTableLatenciesImpl() {

hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsTableQueryMeterImpl.java

@@ -96,4 +96,15 @@ public void updateTableWriteQueryMeter(TableName tableName, long count) {
   public void updateTableWriteQueryMeter(TableName tableName) {
     getOrCreateTableMeter(tableName).updateTableWriteQueryMeter();
   }
+
+  @Override
+  public void deleteTable(TableName tableName) {
+    TableMeters removed = metersByTable.remove(tableName);
+    if (removed == null) {
+      return;
+    }
+    // Also remove the meters from the underlying MetricRegistry so they no longer show up in JMX.
+    metricRegistry.remove(qualifyMetricsName(tableName, TABLE_READ_QUERY_PER_SECOND));
+    metricRegistry.remove(qualifyMetricsName(tableName, TABLE_WRITE_QUERY_PER_SECOND));
+  }
 }

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServer.java

@@ -122,6 +122,16 @@ public MetricsRegionServerWrapper getRegionServerWrapper() {
     return regionServerWrapper;
   }
 
+  /**
+   * Returns the per-table metrics container (may be {@code null} if
+   * {@link #RS_ENABLE_TABLE_METRICS_KEY} is disabled). Exposed so that callers such as
+   * {@link MetricsTableWrapperAggregateImpl} can clean up per-table latency / query meter metrics
+   * when a table leaves the RegionServer.
+   */
+  public RegionServerTableMetrics getRegionServerTableMetrics() {
+    return tableMetrics;
+  }
+
   public void updatePutBatch(TableName tn, long t) {
     if (tableMetrics != null && tn != null) {
       tableMetrics.updatePutBatch(tn, t);

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsTableWrapperAggregateImpl.java

@@ -20,6 +20,7 @@
 import java.io.Closeable;
 import java.io.IOException;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
@@ -43,6 +44,16 @@ public class MetricsTableWrapperAggregateImpl implements MetricsTableWrapperAggr
   private ScheduledFuture<?> tableMetricsUpdateTask;
   private ConcurrentHashMap<TableName, MetricsTableValues> metricsTableMap =
     new ConcurrentHashMap<>();
+  /**
+   * Tables that were seen online in the previous scheduled run. Used to detect tables that
+   * have left this RegionServer so that their per-table latency histograms and query meters
+   * can be cleaned up (HBASE-27486). Per-table latencies and query meters are controlled by
+   * their own switches (hbase.regionserver.enable.table.latencies /
+   * hbase.regionserver.enable.table.queryMeter); this cleanup path runs unconditionally so
+   * that users who enabled either of those switches still get their per-table metrics
+   * released when a table leaves the RegionServer.
+   */
+  private final Set<TableName> lastSeenTables = ConcurrentHashMap.newKeySet();
 
   public MetricsTableWrapperAggregateImpl(final HRegionServer regionServer) {
     this.regionServer = regionServer;
@@ -58,6 +69,30 @@ public class TableMetricsWrapperRunnable implements Runnable {
 
     @Override
     public void run() {
+      // Collect currently online tables first so that the per-table latency / query meter
+      // cleanup path can run before the aggregate metrics collection below.
+      Set<TableName> onlineTables = new HashSet<>();
+      for (Region r : regionServer.getOnlineRegionsLocalContext()) {
+        onlineTables.add(r.getTableDescriptor().getTableName());
+      }
+
+      // ---- Cleanup path for per-table latency histograms & query meters (HBASE-27486) ----
+      // Tables that were seen last round but are no longer online must have their per-table
+      // metrics removed to avoid unbounded growth for short-lived tables.
+      RegionServerTableMetrics rsTableMetrics = regionServer.getMetrics() != null
+        ? regionServer.getMetrics().getRegionServerTableMetrics()
+        : null;
+      if (rsTableMetrics != null) {
+        Set<TableName> goneTables = Sets.newHashSet(lastSeenTables);
+        goneTables.removeAll(onlineTables);
+        for (TableName gone : goneTables) {
+          rsTableMetrics.deleteTable(gone);
+        }
+      }
+      // Refresh lastSeenTables snapshot for next round.
+      lastSeenTables.clear();
+      lastSeenTables.addAll(onlineTables);
+
       Map<TableName, MetricsTableValues> localMetricsTableMap = new HashMap<>();
       for (Region r : regionServer.getOnlineRegionsLocalContext()) {
         TableName tbl = r.getTableDescriptor().getTableName();

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerTableMetrics.java

@@ -105,4 +105,15 @@ public void updateTableWriteQueryMeter(TableName table) {
       queryMeter.updateTableWriteQueryMeter(table);
     }
   }
+
+  /**
+   * Clean up all per-table metrics of the given table. Should be called when the table leaves the
+   * RegionServer (dropped, moved, or all its regions closed) so that the underlying
+   * {@link MetricsTableLatencies} / {@link MetricsTableQueryMeter} do not keep growing forever
+   * (see HBASE-27486 / HBASE-27681).
+   */
+  public void deleteTable(TableName table) {
+    latencies.deleteTable(table.getNameAsString());
+    queryMeter.deleteTable(table);
+  }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsTableLatencies.java

@@ -115,4 +115,100 @@ public void testTableQueryMeterSwitch() {
         MetricsTableQueryMeterImpl.TABLE_WRITE_QUERY_PER_SECOND + "_" + "count"),
       500L, latenciesImpl);
   }
+  /**
+   * Verifies that {@link MetricsTableLatencies#deleteTable(String)} removes every histogram
+   * family (across all metric suffixes) previously registered for the given table on
+   * {@link MetricsTableLatenciesImpl}, while leaving other tables untouched. Also verifies that
+   * re-writing samples for the dropped table lazily re-registers its histograms and that
+   * deleting an unknown table is a no-op (HBASE-27486).
+   */
+  @Test
+  public void testDeleteTableRemovesAllLatencyHistograms() throws IOException {
+    TableName tnKeep = TableName.valueOf("keep_table");
+    TableName tnDrop = TableName.valueOf("drop_table");
+    MetricsTableLatencies latencies =
+      CompatibilitySingletonFactory.getInstance(MetricsTableLatencies.class);
+    assertTrue(latencies instanceof MetricsTableLatenciesImpl,
+      "'latencies' is actually " + latencies.getClass());
+    MetricsTableLatenciesImpl latenciesImpl = (MetricsTableLatenciesImpl) latencies;
+    RegionServerTableMetrics tableMetrics = new RegionServerTableMetrics(false);
+
+    // Every metric family registerd by MetricsTableLatenciesImpl.TableHistograms for a table.
+    String[] families = new String[] {
+      MetricsTableLatencies.GET_TIME,
+      MetricsTableLatencies.PUT_TIME,
+      MetricsTableLatencies.PUT_BATCH_TIME,
+      MetricsTableLatencies.DELETE_TIME,
+      MetricsTableLatencies.DELETE_BATCH_TIME,
+      MetricsTableLatencies.INCREMENT_TIME,
+      MetricsTableLatencies.APPEND_TIME,
+      MetricsTableLatencies.SCAN_TIME,
+      MetricsTableLatencies.SCAN_SIZE,
+      MetricsTableLatencies.CHECK_AND_DELETE_TIME,
+      MetricsTableLatencies.CHECK_AND_PUT_TIME,
+      MetricsTableLatencies.CHECK_AND_MUTATE_TIME };
+
+    // Populate both tables so every histogram family is registered in the underlying
+    // DynamicMetricsRegistry.
+    tableMetrics.updateGet(tnKeep, 100L);
+    tableMetrics.updatePut(tnKeep, 20L);
+    tableMetrics.updatePutBatch(tnKeep, 21L);
+    tableMetrics.updateDelete(tnKeep, 22L);
+    tableMetrics.updateDeleteBatch(tnKeep, 23L);
+    tableMetrics.updateIncrement(tnKeep, 24L);
+    tableMetrics.updateAppend(tnKeep, 25L);
+    tableMetrics.updateScanTime(tnKeep, 26L);
+    tableMetrics.updateScanSize(tnKeep, 27L);
+    tableMetrics.updateCheckAndDelete(tnKeep, 28L);
+    tableMetrics.updateCheckAndPut(tnKeep, 29L);
+    tableMetrics.updateCheckAndMutate(tnKeep, 30L);
+
+    tableMetrics.updateGet(tnDrop, 200L);
+    tableMetrics.updatePut(tnDrop, 40L);
+    tableMetrics.updatePutBatch(tnDrop, 41L);
+    tableMetrics.updateDelete(tnDrop, 42L);
+    tableMetrics.updateDeleteBatch(tnDrop, 43L);
+    tableMetrics.updateIncrement(tnDrop, 44L);
+    tableMetrics.updateAppend(tnDrop, 45L);
+    tableMetrics.updateScanTime(tnDrop, 46L);
+    tableMetrics.updateScanSize(tnDrop, 47L);
+    tableMetrics.updateCheckAndDelete(tnDrop, 48L);
+    tableMetrics.updateCheckAndPut(tnDrop, 49L);
+    tableMetrics.updateCheckAndMutate(tnDrop, 50L);
+
+    // Sanity: every family exists for both tables before deletion.
+    for (String f : families) {
+      assertTrue(
+        HELPER.checkGaugeExists(MetricsTableLatenciesImpl.qualifyMetricsName(tnKeep, f)
+          + "_999th_percentile", latenciesImpl),
+        "keep_table." + f + " should exist before deleteTable");
+      assertTrue(
+        HELPER.checkGaugeExists(MetricsTableLatenciesImpl.qualifyMetricsName(tnDrop, f)
+          + "_999th_percentile", latenciesImpl),
+        "drop_table." + f + " should exist before deleteTable");
+    }
+
+    // Act: drop only tnDrop.
+    latencies.deleteTable(tnDrop.getNameAsString());
+
+    // Assert: all histogram families of tnDrop are gone from the registry, tnKeep is intact.
+    for (String f : families) {
+      assertFalse(
+        HELPER.checkGaugeExists(MetricsTableLatenciesImpl.qualifyMetricsName(tnDrop, f)
+          + "_999th_percentile", latenciesImpl),
+        "drop_table." + f + " should have been removed by deleteTable");
+      assertTrue(
+        HELPER.checkGaugeExists(MetricsTableLatenciesImpl.qualifyMetricsName(tnKeep, f)
+          + "_999th_percentile", latenciesImpl),
+        "keep_table." + f + " must not be affected by deleteTable(drop_table)");
+    }
+
+    // Re-adding samples for the dropped table should lazily re-register its histograms.
+    tableMetrics.updateGet(tnDrop, 999L);
+    HELPER.assertGauge(MetricsTableLatenciesImpl.qualifyMetricsName(tnDrop,
+      MetricsTableLatencies.GET_TIME) + "_999th_percentile", 999L, latenciesImpl);
+
+    // Deleting an unknown table must be a no-op.
+    latencies.deleteTable(TableName.valueOf("never_seen").getNameAsString());
+  }
 }