(phoenix) 01/02: PHOENIX-7874 Addendum Trim redundant per-batch metric work on the indexed write path

[tk...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

tkhurana pushed a commit to branch PHOENIX-7562-feature-new
in repository https://gitbox.apache.org/repos/asf/phoenix.git

commit 10d06ad2f605f2ab8348308a44ad346ec23d8d9a
Author: tkhurana <kh...@gmail.com>
AuthorDate: Tue Sep 15 11:40:58 2026 -0700

    PHOENIX-7874 Addendum Trim redundant per-batch metric work on the indexed write path
    
    Two allocation/CPU cleanups that fire on every indexed batch:
    
    - MetricsIndexerSourceImpl: memoize the resolved per-(table, base-metric)
      MetricHistogram/MutableFastCounter handles instead of rebuilding the
      "base.table" name string and re-resolving the registry handle on every
      update. The (base, table) set is small and bounded and the handles are
      stable for the life of the source, so steady-state per-batch allocation at
      getCounterName drops to zero. Metric names, values, and JMX output are
      unchanged.
    
    - IndexRegionObserver: doPre/doPost emitted updatePreIndexUpdateTime /
      updatePostIndexUpdateTime unconditionally, even when doIndexWritesWithExceptions
      short-circuited with no index updates to write -- recording a near-zero sample
      every batch on tables with no pre/post index work. doIndexWritesWithExceptions
      now returns whether it actually wrote, and the callers only record the timing
      metric when it did. Failure metrics are unchanged.
---
 .../index/metrics/MetricsIndexerSourceImpl.java    | 21 +++++++++++++------
 .../phoenix/hbase/index/IndexRegionObserver.java   | 24 ++++++++++++++--------
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/hbase/index/metrics/MetricsIndexerSourceImpl.java b/phoenix-core-client/src/main/java/org/apache/phoenix/hbase/index/metrics/MetricsIndexerSourceImpl.java
index d6fa330310..e3aea4ae6a 100644
--- a/phoenix-core-client/src/main/java/org/apache/phoenix/hbase/index/metrics/MetricsIndexerSourceImpl.java
+++ b/phoenix-core-client/src/main/java/org/apache/phoenix/hbase/index/metrics/MetricsIndexerSourceImpl.java
@@ -17,6 +17,7 @@
  */
 package org.apache.phoenix.hbase.index.metrics;
 
+import java.util.concurrent.ConcurrentHashMap;
 import org.apache.hadoop.hbase.metrics.BaseSourceImpl;
 import org.apache.hadoop.metrics2.MetricHistogram;
 import org.apache.hadoop.metrics2.lib.MutableFastCounter;
@@ -49,6 +50,14 @@ public class MetricsIndexerSourceImpl extends BaseSourceImpl implements MetricsI
   private final MutableFastCounter preIndexUpdateFailures;
   private final MutableFastCounter postIndexUpdateFailures;
 
+  // Resolved per-(table, base-metric) handles, memoized so the name concat and registry lookup
+  // run once per pair instead of on every per-batch metric update. Keyed tableName -> baseName ->
+  // handle; the (base, table) set is small and bounded, matching the registry's own retention.
+  private final ConcurrentHashMap<String,
+    ConcurrentHashMap<String, MetricHistogram>> tableHistograms = new ConcurrentHashMap<>();
+  private final ConcurrentHashMap<String,
+    ConcurrentHashMap<String, MutableFastCounter>> tableCounters = new ConcurrentHashMap<>();
+
   public MetricsIndexerSourceImpl() {
     this(METRICS_NAME, METRICS_DESCRIPTION, METRICS_CONTEXT, METRICS_JMX_CONTEXT);
   }
@@ -225,15 +234,15 @@ public class MetricsIndexerSourceImpl extends BaseSourceImpl implements MetricsI
   }
 
   private void incrementTableSpecificCounter(String baseCounterName, String tableName) {
-    MutableFastCounter indexSpecificCounter =
-      getMetricsRegistry().getCounter(getCounterName(baseCounterName, tableName), 0);
-    indexSpecificCounter.incr();
+    tableCounters.computeIfAbsent(tableName, k -> new ConcurrentHashMap<>())
+      .computeIfAbsent(baseCounterName,
+        b -> getMetricsRegistry().getCounter(getCounterName(b, tableName), 0))
+      .incr();
   }
 
   private void incrementTableSpecificHistogram(String baseCounterName, String tableName, long t) {
-    MetricHistogram tableSpecificHistogram =
-      getMetricsRegistry().getHistogram(getCounterName(baseCounterName, tableName));
-    tableSpecificHistogram.add(t);
+    tableHistograms.computeIfAbsent(tableName, k -> new ConcurrentHashMap<>()).computeIfAbsent(
+      baseCounterName, b -> getMetricsRegistry().getHistogram(getCounterName(b, tableName))).add(t);
   }
 
   private String getCounterName(String baseCounterName, String tableName) {
diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
index a762a77467..dd887cc73f 100644
--- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
+++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
@@ -3052,9 +3052,12 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
         throw new DoNotRetryIOException(
           "Simulating the last (i.e., post) index table write failure");
       }
-      doIndexWritesWithExceptions(context, true);
-      metricSource.updatePostIndexUpdateTime(dataTableName,
-        EnvironmentEdgeManager.currentTimeMillis() - start);
+      // Only record the post-index-update time when there were actually index updates to write;
+      // otherwise every batch with no post index updates emits a near-zero sample.
+      if (doIndexWritesWithExceptions(context, true)) {
+        metricSource.updatePostIndexUpdateTime(dataTableName,
+          EnvironmentEdgeManager.currentTimeMillis() - start);
+      }
     } catch (Throwable e) {
       metricSource.updatePostIndexUpdateFailureTime(dataTableName,
         EnvironmentEdgeManager.currentTimeMillis() - start);
@@ -3063,14 +3066,15 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
     }
   }
 
-  private void doIndexWritesWithExceptions(BatchMutateContext context, boolean post)
+  /** Returns true if index updates were actually written, false if there was no work to do. */
+  private boolean doIndexWritesWithExceptions(BatchMutateContext context, boolean post)
     throws IOException {
     ListMultimap<HTableInterfaceReference, Mutation> indexUpdates =
       post ? context.postIndexUpdates : context.preIndexUpdates;
     // short circuit, if we don't need to do any work
 
     if (context == null || indexUpdates == null || indexUpdates.isEmpty()) {
-      return;
+      return false;
     }
 
     // get the current span, or just use a null-span to avoid a bunch of if statements
@@ -3088,6 +3092,7 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
         preWriter.write(indexUpdates, false, context.clientVersion);
       }
     }
+    return true;
   }
 
   private void removePendingRows(BatchMutateContext context) {
@@ -3107,9 +3112,12 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
         throw new DoNotRetryIOException(
           "Simulating the first (i.e., pre) index table write failure");
       }
-      doIndexWritesWithExceptions(context, false);
-      metricSource.updatePreIndexUpdateTime(dataTableName,
-        EnvironmentEdgeManager.currentTimeMillis() - start);
+      // Only record the pre-index-update time when there were actually index updates to write;
+      // otherwise every batch with no pre index updates emits a near-zero sample.
+      if (doIndexWritesWithExceptions(context, false)) {
+        metricSource.updatePreIndexUpdateTime(dataTableName,
+          EnvironmentEdgeManager.currentTimeMillis() - start);
+      }
     } catch (Throwable e) {
       metricSource.updatePreIndexUpdateFailureTime(dataTableName,
         EnvironmentEdgeManager.currentTimeMillis() - start);