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 <[email protected]> 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);
