This is an automated email from the ASF dual-hosted git repository.
liuxiaocs7 pushed a commit to branch branch-3
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-3 by this push:
new 40e873d457c HBASE-28123 TableRequests metrics broken after altering
table (#7855)
40e873d457c is described below
commit 40e873d457ce88db6513772cc1902037ff1509eb
Author: Xiao Liu <[email protected]>
AuthorDate: Thu Jun 18 18:37:33 2026 +0800
HBASE-28123 TableRequests metrics broken after altering table (#7855)
Signed-off-by: Duo Zhang <[email protected]>
---
.../impl/GlobalMetricRegistriesAdapter.java | 20 ++--
.../impl/TestGlobalMetricRegistriesAdapter.java | 114 +++++++++++++++++++++
.../hbase/regionserver/MetricsRegionServer.java | 9 --
.../regionserver/metrics/MetricsTableRequests.java | 41 +++-----
.../regionserver/TestMetricsRegionServer.java | 4 +-
.../regionserver/TestMetricsUserAggregate.java | 4 +-
.../{ => metrics}/TestMetricsTableRequests.java | 34 ++++--
7 files changed, 165 insertions(+), 61 deletions(-)
diff --git
a/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/metrics/impl/GlobalMetricRegistriesAdapter.java
b/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/metrics/impl/GlobalMetricRegistriesAdapter.java
index 6ae9c68edca..99b80e2147b 100644
---
a/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/metrics/impl/GlobalMetricRegistriesAdapter.java
+++
b/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/metrics/impl/GlobalMetricRegistriesAdapter.java
@@ -63,15 +63,17 @@ public final class GlobalMetricRegistriesAdapter {
private static final Logger LOG =
LoggerFactory.getLogger(GlobalMetricRegistriesAdapter.class);
private class MetricsSourceAdapter implements MetricsSource {
- private final MetricRegistry registry;
+ private final MetricRegistryInfo info;
- MetricsSourceAdapter(MetricRegistry registry) {
- this.registry = registry;
+ MetricsSourceAdapter(MetricRegistryInfo info) {
+ this.info = info;
}
@Override
public void getMetrics(MetricsCollector collector, boolean all) {
- metricsAdapter.snapshotAllMetrics(registry, collector);
+ Optional<MetricRegistry> registryOpt =
MetricRegistries.global().get(info);
+ registryOpt
+ .ifPresent(metricRegistry ->
metricsAdapter.snapshotAllMetrics(metricRegistry, collector));
}
}
@@ -115,7 +117,7 @@ public final class GlobalMetricRegistriesAdapter {
for (MetricRegistry registry : registries) {
MetricRegistryInfo info = registry.getMetricRegistryInfo();
- LOG.trace("MetricRegistryInfo : " + info.getMetricsName());
+ LOG.trace("MetricRegistryInfo : {}", info.getMetricsName());
if (info.isExistingSource()) {
// If there is an already existing BaseSource for this MetricRegistry,
skip it here. These
// types of registries are there only due to existing BaseSource
implementations in the
@@ -128,10 +130,10 @@ public final class GlobalMetricRegistriesAdapter {
if (!registeredSources.containsKey(info)) {
if (LOG.isDebugEnabled()) {
- LOG.debug("Registering adapter for the MetricRegistry: " +
info.getMetricsJmxContext());
+ LOG.debug("Registering adapter for the MetricRegistry: {}",
info.getMetricsJmxContext());
}
// register this as a MetricSource under different JMX Context'es.
- MetricsSourceAdapter adapter = new MetricsSourceAdapter(registry);
+ MetricsSourceAdapter adapter = new MetricsSourceAdapter(info);
if (LOG.isDebugEnabled()) {
LOG.debug("Registering {} {}", info.getMetricsJmxContext(),
info.getMetricsDescription());
}
@@ -150,9 +152,9 @@ public final class GlobalMetricRegistriesAdapter {
Entry<MetricRegistryInfo, MetricsSourceAdapter> entry = it.next();
MetricRegistryInfo info = entry.getKey();
Optional<MetricRegistry> found = MetricRegistries.global().get(info);
- if (!found.isPresent()) {
+ if (found.isEmpty()) {
if (LOG.isDebugEnabled()) {
- LOG.debug("Removing adapter for the MetricRegistry: " +
info.getMetricsJmxContext());
+ LOG.debug("Removing adapter for the MetricRegistry: {}",
info.getMetricsJmxContext());
}
synchronized (DefaultMetricsSystem.instance()) {
DefaultMetricsSystem.instance().unregisterSource(info.getMetricsJmxContext());
diff --git
a/hbase-hadoop-compat/src/test/java/org/apache/hadoop/hbase/metrics/impl/TestGlobalMetricRegistriesAdapter.java
b/hbase-hadoop-compat/src/test/java/org/apache/hadoop/hbase/metrics/impl/TestGlobalMetricRegistriesAdapter.java
new file mode 100644
index 00000000000..aa964926d93
--- /dev/null
+++
b/hbase-hadoop-compat/src/test/java/org/apache/hadoop/hbase/metrics/impl/TestGlobalMetricRegistriesAdapter.java
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.metrics.impl;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.lang.reflect.Method;
+import java.util.Collection;
+import org.apache.hadoop.hbase.metrics.MetricRegistries;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
+import org.apache.hadoop.hbase.testclassification.MetricsTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.apache.hadoop.metrics2.AbstractMetric;
+import org.apache.hadoop.metrics2.MetricsRecord;
+import org.apache.hadoop.metrics2.impl.MetricsExportHelper;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+@Tag(MetricsTests.TAG)
+@Tag(SmallTests.TAG)
+public class TestGlobalMetricRegistriesAdapter {
+
+ private static final String METRICS_NAME =
"TestGlobalMetricRegistriesAdapter";
+ private static final String COUNTER_NAME = "requests";
+ private static final MetricRegistryInfo INFO =
+ new MetricRegistryInfo(METRICS_NAME, "Metrics for
GlobalMetricRegistriesAdapter tests",
+ "RegionServer,sub=" + METRICS_NAME, "regionserver", false);
+
+ private GlobalMetricRegistriesAdapter adapter;
+
+ @BeforeEach
+ public void setUp() {
+ MetricRegistries.global().clear();
+ DefaultMetricsSystem.shutdown();
+ DefaultMetricsSystem.initialize("globalMetricRegistriesAdapterTest");
+ DefaultMetricsSystem.instance().start();
+ adapter = GlobalMetricRegistriesAdapter.init();
+ }
+
+ @AfterEach
+ public void tearDown() {
+ MetricRegistries.global().clear();
+ if (adapter != null) {
+ // First unregister sources for the cleared registries, then stop the
adapter's executor.
+ runAdapter();
+ adapter.stop();
+ runAdapter();
+ }
+ DefaultMetricsSystem.shutdown();
+ }
+
+ @Test
+ public void testExportUsesRecreatedMetricRegistry() {
+ MetricRegistry registryBefore = MetricRegistries.global().create(INFO);
+ registryBefore.counter(COUNTER_NAME).increment(1);
+ runAdapter();
+
+ assertEquals(1, getExportedCounterValue());
+
+ assertTrue(MetricRegistries.global().remove(INFO));
+ MetricRegistry registryAfter = MetricRegistries.global().create(INFO);
+ assertNotSame(registryBefore, registryAfter);
+ registryAfter.counter(COUNTER_NAME).increment(2);
+ runAdapter();
+
+ assertEquals(2, getExportedCounterValue());
+ }
+
+ private void runAdapter() {
+ try {
+ // Trigger the adapter synchronously so this small test does not wait
for the 10s scheduler.
+ Method doRun =
GlobalMetricRegistriesAdapter.class.getDeclaredMethod("doRun");
+ doRun.setAccessible(true);
+ doRun.invoke(adapter);
+ } catch (ReflectiveOperationException e) {
+ throw new AssertionError("Failed to run GlobalMetricRegistriesAdapter",
e);
+ }
+ }
+
+ private long getExportedCounterValue() {
+ Collection<MetricsRecord> exportedMetrics = MetricsExportHelper.export();
+ for (MetricsRecord exported : exportedMetrics) {
+ if (exported.name().equals(INFO.getMetricsName())) {
+ for (AbstractMetric metric : exported.metrics()) {
+ if (metric.name().equals("Requests")) {
+ return metric.value().longValue();
+ }
+ }
+ }
+ }
+ return -1;
+ }
+}
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServer.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServer.java
index 0de8e96962c..5eec2595abc 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServer.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServer.java
@@ -282,15 +282,6 @@ public class MetricsRegionServer {
}
}
- public void updateWriteQueryMeter(HRegion region) {
- if (region.getMetricsTableRequests() != null) {
- region.getMetricsTableRequests().updateTableWriteQueryMeter();
- }
- if (serverWriteQueryMeter != null) {
- serverWriteQueryMeter.mark();
- }
- }
-
public void incrScannerLeaseExpired() {
serverSource.incrScannerLeaseExpired();
}
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/MetricsTableRequests.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/MetricsTableRequests.java
index 357a379357f..34ba725dde3 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/MetricsTableRequests.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/MetricsTableRequests.java
@@ -110,14 +110,14 @@ public class MetricsTableRequests {
private MetricRegistryInfo registryInfo;
private boolean enableTableLatenciesMetrics;
- private boolean enabTableQueryMeterMetrics;
+ private boolean enableTableQueryMeterMetrics;
public boolean isEnableTableLatenciesMetrics() {
return enableTableLatenciesMetrics;
}
- public boolean isEnabTableQueryMeterMetrics() {
- return enabTableQueryMeterMetrics;
+ public boolean isEnableTableQueryMeterMetrics() {
+ return enableTableQueryMeterMetrics;
}
public MetricsTableRequests(TableName tableName, Configuration conf) {
@@ -129,9 +129,9 @@ public class MetricsTableRequests {
this.conf = conf;
enableTableLatenciesMetrics =
this.conf.getBoolean(ENABLE_TABLE_LATENCIES_METRICS_KEY,
ENABLE_TABLE_LATENCIES_METRICS_DEFAULT);
- enabTableQueryMeterMetrics =
this.conf.getBoolean(ENABLE_TABLE_QUERY_METER_METRICS_KEY,
+ enableTableQueryMeterMetrics =
this.conf.getBoolean(ENABLE_TABLE_QUERY_METER_METRICS_KEY,
ENABLE_TABLE_QUERY_METER_METRICS_KEY_DEFAULT);
- if (enableTableLatenciesMetrics || enabTableQueryMeterMetrics) {
+ if (enableTableLatenciesMetrics || enableTableQueryMeterMetrics) {
registry = createRegistryForTableRequests();
if (enableTableLatenciesMetrics) {
getTimeHistogram = registry.histogram(GET_TIME);
@@ -155,7 +155,7 @@ public class MetricsTableRequests {
scanBlockBytesScanned =
registry.histogram(SCAN_BLOCK_BYTES_SCANNED_KEY);
}
- if (enabTableQueryMeterMetrics) {
+ if (enableTableQueryMeterMetrics) {
readMeter = registry.meter(TABLE_READ_QUERY_PER_SECOND);
writeMeter = registry.meter(TABLE_WRITE_QUERY_PER_SECOND);
}
@@ -173,7 +173,7 @@ public class MetricsTableRequests {
}
public void removeRegistry() {
- if (enableTableLatenciesMetrics || enabTableQueryMeterMetrics) {
+ if (enableTableLatenciesMetrics || enableTableQueryMeterMetrics) {
MetricRegistries.global().remove(registry.getMetricRegistryInfo());
}
}
@@ -327,41 +327,26 @@ public class MetricsTableRequests {
* @param count Number of occurrences to record
*/
public void updateTableReadQueryMeter(long count) {
- if (isEnabTableQueryMeterMetrics()) {
+ if (isEnableTableQueryMeterMetrics()) {
readMeter.mark(count);
}
}
- /**
- * Update table read QPS
- */
- public void updateTableReadQueryMeter() {
- if (isEnabTableQueryMeterMetrics()) {
- readMeter.mark();
- }
- }
-
/**
* Update table write QPS
* @param count Number of occurrences to record
*/
public void updateTableWriteQueryMeter(long count) {
- if (isEnabTableQueryMeterMetrics()) {
+ if (isEnableTableQueryMeterMetrics()) {
writeMeter.mark(count);
}
}
- /**
- * Update table write QPS
- */
- public void updateTableWriteQueryMeter() {
- if (isEnabTableQueryMeterMetrics()) {
- writeMeter.mark();
- }
+ MetricRegistryInfo getMetricRegistryInfo() {
+ return registryInfo;
}
- // Visible for testing
- public MetricRegistryInfo getMetricRegistryInfo() {
- return registryInfo;
+ MetricRegistry getMetricRegistry() {
+ return registry;
}
}
diff --git
a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegionServer.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegionServer.java
index 00013851e20..d5b1ecf643d 100644
---
a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegionServer.java
+++
b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegionServer.java
@@ -166,7 +166,7 @@ public class TestMetricsRegionServer {
MetricsTableRequests metricsTableRequests =
mock(MetricsTableRequests.class);
when(region.getMetricsTableRequests()).thenReturn(metricsTableRequests);
when(metricsTableRequests.isEnableTableLatenciesMetrics()).thenReturn(false);
-
when(metricsTableRequests.isEnabTableQueryMeterMetrics()).thenReturn(false);
+
when(metricsTableRequests.isEnableTableQueryMeterMetrics()).thenReturn(false);
for (int i = 0; i < 12; i++) {
rsm.updateAppend(region, 12, 120);
rsm.updateAppend(region, 1002, 10020);
@@ -306,7 +306,7 @@ public class TestMetricsRegionServer {
MetricsTableRequests metricsTableRequests =
mock(MetricsTableRequests.class);
when(region.getMetricsTableRequests()).thenReturn(metricsTableRequests);
when(metricsTableRequests.isEnableTableLatenciesMetrics()).thenReturn(false);
-
when(metricsTableRequests.isEnabTableQueryMeterMetrics()).thenReturn(false);
+
when(metricsTableRequests.isEnableTableQueryMeterMetrics()).thenReturn(false);
Configuration conf = new Configuration(false);
// disable
rsm.updateReadQueryMeter(region, 500L);
diff --git
a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsUserAggregate.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsUserAggregate.java
index 18b4ff74563..a845c01e40c 100644
---
a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsUserAggregate.java
+++
b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsUserAggregate.java
@@ -68,7 +68,7 @@ public class TestMetricsUserAggregate {
MetricsTableRequests metricsTableRequests =
mock(MetricsTableRequests.class);
when(region.getMetricsTableRequests()).thenReturn(metricsTableRequests);
when(metricsTableRequests.isEnableTableLatenciesMetrics()).thenReturn(false);
-
when(metricsTableRequests.isEnabTableQueryMeterMetrics()).thenReturn(false);
+
when(metricsTableRequests.isEnableTableQueryMeterMetrics()).thenReturn(false);
for (int i = 0; i < 10; i++) {
rsm.updateGet(region, 10, 10);
}
@@ -146,7 +146,7 @@ public class TestMetricsUserAggregate {
MetricsTableRequests metricsTableRequests =
mock(MetricsTableRequests.class);
when(region.getMetricsTableRequests()).thenReturn(metricsTableRequests);
when(metricsTableRequests.isEnableTableLatenciesMetrics()).thenReturn(false);
-
when(metricsTableRequests.isEnabTableQueryMeterMetrics()).thenReturn(false);
+
when(metricsTableRequests.isEnableTableQueryMeterMetrics()).thenReturn(false);
rsm.updateGet(region, 10, 100);
return null;
}
diff --git
a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsTableRequests.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsTableRequests.java
similarity index 82%
rename from
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsTableRequests.java
rename to
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsTableRequests.java
index a3463f59126..48390791b66 100644
---
a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsTableRequests.java
+++
b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsTableRequests.java
@@ -15,10 +15,11 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.hadoop.hbase.regionserver;
+package org.apache.hadoop.hbase.regionserver.metrics;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.Optional;
@@ -31,12 +32,12 @@ import org.apache.hadoop.hbase.metrics.MetricRegistryInfo;
import org.apache.hadoop.hbase.metrics.Snapshot;
import org.apache.hadoop.hbase.metrics.impl.DropwizardMeter;
import org.apache.hadoop.hbase.metrics.impl.HistogramImpl;
-import org.apache.hadoop.hbase.regionserver.metrics.MetricsTableRequests;
import org.apache.hadoop.hbase.testclassification.RegionServerTests;
import org.apache.hadoop.hbase.testclassification.SmallTests;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
@Tag(RegionServerTests.TAG)
@Tag(SmallTests.TAG)
@@ -53,10 +54,6 @@ public class TestMetricsTableRequests {
TableName tn2 = TableName.valueOf("table2");
MetricsTableRequests requests1 = new MetricsTableRequests(tn1, new
Configuration());
MetricsTableRequests requests2 = new MetricsTableRequests(tn2, new
Configuration());
- assertTrue(requests1 instanceof MetricsTableRequests,
- "'requests' is actually " + requests1.getClass());
- assertTrue(requests2 instanceof MetricsTableRequests,
- "'requests' is actually " + requests2.getClass());
MetricRegistryInfo info1 = requests1.getMetricRegistryInfo();
MetricRegistryInfo info2 = requests2.getMetricRegistryInfo();
@@ -102,8 +99,6 @@ public class TestMetricsTableRequests {
// disable
assertFalse(enableTableQueryMeter);
MetricsTableRequests requests = new MetricsTableRequests(tn1, conf);
- assertTrue(requests instanceof MetricsTableRequests,
- "'requests' is actually " + requests.getClass());
MetricRegistryInfo info = requests.getMetricRegistryInfo();
Optional<MetricRegistry> registry = MetricRegistries.global().get(info);
@@ -119,8 +114,6 @@ public class TestMetricsTableRequests {
MetricsTableRequests.ENABLE_TABLE_QUERY_METER_METRICS_KEY_DEFAULT);
assertTrue(enableTableQueryMeter);
requests = new MetricsTableRequests(tn1, conf);
- assertTrue(requests instanceof MetricsTableRequests,
- "'requests' is actually " + requests.getClass());
info = requests.getMetricRegistryInfo();
registry = MetricRegistries.global().get(info);
@@ -128,6 +121,25 @@ public class TestMetricsTableRequests {
requests.updateTableReadQueryMeter(500L);
read = registry.get().get("tableReadQueryPerSecond");
assertTrue(read.isPresent());
- assertEquals(((DropwizardMeter) read.get()).getCount(), 500);
+ assertEquals(500, ((DropwizardMeter) read.get()).getCount());
+ }
+
+ @Test
+ public void testSameRegistryInstanceRefCounting(TestInfo testInfo) {
+ TableName tn1 =
TableName.valueOf(testInfo.getTestMethod().get().getName());
+
+ // create registry twice, should return same instance due to ref-counting
+ MetricsTableRequests requests1 = new MetricsTableRequests(tn1, new
Configuration());
+ MetricsTableRequests requests2 = new MetricsTableRequests(tn1, new
Configuration());
+
+ MetricRegistry metricRegistry1 = requests1.getMetricRegistry();
+ MetricRegistry metricRegistry2 = requests2.getMetricRegistry();
+ assertSame(metricRegistry1, metricRegistry2);
+
+ MetricRegistryInfo registryInfo = metricRegistry1.getMetricRegistryInfo();
+ MetricRegistries.global().remove(registryInfo);
+ assertTrue(MetricRegistries.global().get(registryInfo).isPresent());
+ MetricRegistries.global().remove(registryInfo);
+ assertFalse(MetricRegistries.global().get(registryInfo).isPresent());
}
}
