Skip to content

Commit b2e3649

Browse files
liuxiaocs7liuxiaocs7
committed
HBASE-28123 TableRequests metrics broken after altering table
1 parent 7f1ea6a commit b2e3649

File tree

7 files changed

+254
-73
lines changed

7 files changed

+254
-73
lines changed

‎hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/metrics/impl/GlobalMetricRegistriesAdapter.java‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -63,15 +63,17 @@ public final class GlobalMetricRegistriesAdapter {
6363
private static final Logger LOG = LoggerFactory.getLogger(GlobalMetricRegistriesAdapter.class);
6464

6565
private class MetricsSourceAdapter implements MetricsSource {
66-
private final MetricRegistry registry;
66+
private final MetricRegistryInfo info;
6767

68-
MetricsSourceAdapter(MetricRegistry registry) {
69-
this.registry = registry;
68+
MetricsSourceAdapter(MetricRegistryInfo info) {
69+
this.info = info;
7070
}
7171

7272
@Override
7373
public void getMetrics(MetricsCollector collector, boolean all) {
74-
metricsAdapter.snapshotAllMetrics(registry, collector);
74+
Optional<MetricRegistry> registryOpt = MetricRegistries.global().get(info);
75+
registryOpt
76+
.ifPresent(metricRegistry -> metricsAdapter.snapshotAllMetrics(metricRegistry, collector));
7577
}
7678
}
7779

@@ -115,7 +117,7 @@ private void doRun() {
115117
for (MetricRegistry registry : registries) {
116118
MetricRegistryInfo info = registry.getMetricRegistryInfo();
117119

118-
LOG.trace("MetricRegistryInfo : " + info.getMetricsName());
120+
LOG.trace("MetricRegistryInfo : {}", info.getMetricsName());
119121
if (info.isExistingSource()) {
120122
// If there is an already existing BaseSource for this MetricRegistry, skip it here. These
121123
// types of registries are there only due to existing BaseSource implementations in the
@@ -128,10 +130,10 @@ private void doRun() {
128130

129131
if (!registeredSources.containsKey(info)) {
130132
if (LOG.isDebugEnabled()) {
131-
LOG.debug("Registering adapter for the MetricRegistry: " + info.getMetricsJmxContext());
133+
LOG.debug("Registering adapter for the MetricRegistry: {}", info.getMetricsJmxContext());
132134
}
133135
// register this as a MetricSource under different JMX Context'es.
134-
MetricsSourceAdapter adapter = new MetricsSourceAdapter(registry);
136+
MetricsSourceAdapter adapter = new MetricsSourceAdapter(info);
135137
LOG.info("Registering " + info.getMetricsJmxContext() + " " + info.getMetricsDescription());
136138
DefaultMetricsSystem.instance().register(info.getMetricsJmxContext(),
137139
info.getMetricsDescription(), adapter);
@@ -148,9 +150,9 @@ private void doRun() {
148150
Entry<MetricRegistryInfo, MetricsSourceAdapter> entry = it.next();
149151
MetricRegistryInfo info = entry.getKey();
150152
Optional<MetricRegistry> found = MetricRegistries.global().get(info);
151-
if (!found.isPresent()) {
153+
if (found.isEmpty()) {
152154
if (LOG.isDebugEnabled()) {
153-
LOG.debug("Removing adapter for the MetricRegistry: " + info.getMetricsJmxContext());
155+
LOG.debug("Removing adapter for the MetricRegistry: {}", info.getMetricsJmxContext());
154156
}
155157
synchronized (DefaultMetricsSystem.instance()) {
156158
DefaultMetricsSystem.instance().unregisterSource(info.getMetricsJmxContext());

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

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -287,15 +287,6 @@ public void updateWriteQueryMeter(HRegion region, long count) {
287287
}
288288
}
289289

290-
public void updateWriteQueryMeter(HRegion region) {
291-
if (region.getMetricsTableRequests() != null) {
292-
region.getMetricsTableRequests().updateTableWriteQueryMeter();
293-
}
294-
if (serverWriteQueryMeter != null) {
295-
serverWriteQueryMeter.mark();
296-
}
297-
}
298-
299290
public void incrScannerLeaseExpired() {
300291
serverSource.incrScannerLeaseExpired();
301292
}

‎hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/MetricsTableRequests.java‎

Lines changed: 18 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
*/
1818
package org.apache.hadoop.hbase.regionserver.metrics;
1919

20+
import com.google.errorprone.annotations.RestrictedApi;
2021
import org.apache.hadoop.conf.Configuration;
2122
import org.apache.hadoop.hbase.TableName;
2223
import org.apache.hadoop.hbase.metrics.Counter;
@@ -110,14 +111,14 @@ public class MetricsTableRequests {
110111
private MetricRegistryInfo registryInfo;
111112

112113
private boolean enableTableLatenciesMetrics;
113-
private boolean enabTableQueryMeterMetrics;
114+
private boolean enableTableQueryMeterMetrics;
114115

115116
public boolean isEnableTableLatenciesMetrics() {
116117
return enableTableLatenciesMetrics;
117118
}
118119

119-
public boolean isEnabTableQueryMeterMetrics() {
120-
return enabTableQueryMeterMetrics;
120+
public boolean isEnableTableQueryMeterMetrics() {
121+
return enableTableQueryMeterMetrics;
121122
}
122123

123124
public MetricsTableRequests(TableName tableName, Configuration conf) {
@@ -129,9 +130,9 @@ private void init(TableName tableName, Configuration conf) {
129130
this.conf = conf;
130131
enableTableLatenciesMetrics = this.conf.getBoolean(ENABLE_TABLE_LATENCIES_METRICS_KEY,
131132
ENABLE_TABLE_LATENCIES_METRICS_DEFAULT);
132-
enabTableQueryMeterMetrics = this.conf.getBoolean(ENABLE_TABLE_QUERY_METER_METRICS_KEY,
133+
enableTableQueryMeterMetrics = this.conf.getBoolean(ENABLE_TABLE_QUERY_METER_METRICS_KEY,
133134
ENABLE_TABLE_QUERY_METER_METRICS_KEY_DEFAULT);
134-
if (enableTableLatenciesMetrics || enabTableQueryMeterMetrics) {
135+
if (enableTableLatenciesMetrics || enableTableQueryMeterMetrics) {
135136
registry = createRegistryForTableRequests();
136137
if (enableTableLatenciesMetrics) {
137138
getTimeHistogram = registry.histogram(GET_TIME);
@@ -155,7 +156,7 @@ private void init(TableName tableName, Configuration conf) {
155156
scanBlockBytesScanned = registry.histogram(SCAN_BLOCK_BYTES_SCANNED_KEY);
156157
}
157158

158-
if (enabTableQueryMeterMetrics) {
159+
if (enableTableQueryMeterMetrics) {
159160
readMeter = registry.meter(TABLE_READ_QUERY_PER_SECOND);
160161
writeMeter = registry.meter(TABLE_WRITE_QUERY_PER_SECOND);
161162
}
@@ -173,7 +174,7 @@ private MetricRegistryInfo createRegistryInfoForTableRequests() {
173174
}
174175

175176
public void removeRegistry() {
176-
if (enableTableLatenciesMetrics || enabTableQueryMeterMetrics) {
177+
if (enableTableLatenciesMetrics || enableTableQueryMeterMetrics) {
177178
MetricRegistries.global().remove(registry.getMetricRegistryInfo());
178179
}
179180
}
@@ -327,41 +328,30 @@ public void updateCheckAndMutate(long time, long blockBytesScanned) {
327328
* @param count Number of occurrences to record
328329
*/
329330
public void updateTableReadQueryMeter(long count) {
330-
if (isEnabTableQueryMeterMetrics()) {
331+
if (isEnableTableQueryMeterMetrics()) {
331332
readMeter.mark(count);
332333
}
333334
}
334335

335-
/**
336-
* Update table read QPS
337-
*/
338-
public void updateTableReadQueryMeter() {
339-
if (isEnabTableQueryMeterMetrics()) {
340-
readMeter.mark();
341-
}
342-
}
343-
344336
/**
345337
* Update table write QPS
346338
* @param count Number of occurrences to record
347339
*/
348340
public void updateTableWriteQueryMeter(long count) {
349-
if (isEnabTableQueryMeterMetrics()) {
341+
if (isEnableTableQueryMeterMetrics()) {
350342
writeMeter.mark(count);
351343
}
352344
}
353345

354-
/**
355-
* Update table write QPS
356-
*/
357-
public void updateTableWriteQueryMeter() {
358-
if (isEnabTableQueryMeterMetrics()) {
359-
writeMeter.mark();
360-
}
361-
}
362-
363-
// Visible for testing
346+
@RestrictedApi(explanation = "Should only be called in TestMetricsTableRequests", link = "",
347+
allowedOnPath = ".*/TestMetricsTableRequests.java")
364348
public MetricRegistryInfo getMetricRegistryInfo() {
365349
return registryInfo;
366350
}
351+
352+
@RestrictedApi(explanation = "Should only be called in TestMetricsTableRequests", link = "",
353+
allowedOnPath = ".*/TestMetricsTableRequests.java")
354+
public MetricRegistry getMetricRegistry() {
355+
return registry;
356+
}
367357
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ public void testSlowCount() {
171171
MetricsTableRequests metricsTableRequests = mock(MetricsTableRequests.class);
172172
when(region.getMetricsTableRequests()).thenReturn(metricsTableRequests);
173173
when(metricsTableRequests.isEnableTableLatenciesMetrics()).thenReturn(false);
174-
when(metricsTableRequests.isEnabTableQueryMeterMetrics()).thenReturn(false);
174+
when(metricsTableRequests.isEnableTableQueryMeterMetrics()).thenReturn(false);
175175
for (int i = 0; i < 12; i++) {
176176
rsm.updateAppend(region, 12, 120);
177177
rsm.updateAppend(region, 1002, 10020);
@@ -311,7 +311,7 @@ public void testTableQueryMeterSwitch() {
311311
MetricsTableRequests metricsTableRequests = mock(MetricsTableRequests.class);
312312
when(region.getMetricsTableRequests()).thenReturn(metricsTableRequests);
313313
when(metricsTableRequests.isEnableTableLatenciesMetrics()).thenReturn(false);
314-
when(metricsTableRequests.isEnabTableQueryMeterMetrics()).thenReturn(false);
314+
when(metricsTableRequests.isEnableTableQueryMeterMetrics()).thenReturn(false);
315315
Configuration conf = new Configuration(false);
316316
// disable
317317
rsm.updateReadQueryMeter(region, 500L);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ private void doOperations() {
7373
MetricsTableRequests metricsTableRequests = mock(MetricsTableRequests.class);
7474
when(region.getMetricsTableRequests()).thenReturn(metricsTableRequests);
7575
when(metricsTableRequests.isEnableTableLatenciesMetrics()).thenReturn(false);
76-
when(metricsTableRequests.isEnabTableQueryMeterMetrics()).thenReturn(false);
76+
when(metricsTableRequests.isEnableTableQueryMeterMetrics()).thenReturn(false);
7777
for (int i = 0; i < 10; i++) {
7878
rsm.updateGet(region, 10, 10);
7979
}
@@ -151,7 +151,7 @@ public Void run() {
151151
MetricsTableRequests metricsTableRequests = mock(MetricsTableRequests.class);
152152
when(region.getMetricsTableRequests()).thenReturn(metricsTableRequests);
153153
when(metricsTableRequests.isEnableTableLatenciesMetrics()).thenReturn(false);
154-
when(metricsTableRequests.isEnabTableQueryMeterMetrics()).thenReturn(false);
154+
when(metricsTableRequests.isEnableTableQueryMeterMetrics()).thenReturn(false);
155155
rsm.updateGet(region, 10, 100);
156156
return null;
157157
}

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsTableRequests.java renamed to hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsTableRequests.java

Lines changed: 30 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,14 @@
1515
* See the License for the specific language governing permissions and
1616
* limitations under the License.
1717
*/
18-
package org.apache.hadoop.hbase.regionserver;
18+
package org.apache.hadoop.hbase.regionserver.metrics;
1919

20-
import static org.junit.Assert.assertEquals;
21-
import static org.junit.Assert.assertFalse;
22-
import static org.junit.Assert.assertTrue;
20+
import static org.junit.jupiter.api.Assertions.assertEquals;
21+
import static org.junit.jupiter.api.Assertions.assertFalse;
22+
import static org.junit.jupiter.api.Assertions.assertTrue;
2323

2424
import java.util.Optional;
2525
import org.apache.hadoop.conf.Configuration;
26-
import org.apache.hadoop.hbase.HBaseClassTestRule;
2726
import org.apache.hadoop.hbase.TableName;
2827
import org.apache.hadoop.hbase.metrics.Metric;
2928
import org.apache.hadoop.hbase.metrics.MetricRegistries;
@@ -32,30 +31,22 @@
3231
import org.apache.hadoop.hbase.metrics.Snapshot;
3332
import org.apache.hadoop.hbase.metrics.impl.DropwizardMeter;
3433
import org.apache.hadoop.hbase.metrics.impl.HistogramImpl;
35-
import org.apache.hadoop.hbase.regionserver.metrics.MetricsTableRequests;
3634
import org.apache.hadoop.hbase.testclassification.RegionServerTests;
3735
import org.apache.hadoop.hbase.testclassification.SmallTests;
38-
import org.junit.ClassRule;
39-
import org.junit.Test;
40-
import org.junit.experimental.categories.Category;
36+
import org.junit.jupiter.api.Tag;
37+
import org.junit.jupiter.api.Test;
38+
import org.junit.jupiter.api.TestInfo;
4139

42-
@Category({ RegionServerTests.class, SmallTests.class })
40+
@Tag(RegionServerTests.TAG)
41+
@Tag(SmallTests.TAG)
4342
public class TestMetricsTableRequests {
4443

45-
@ClassRule
46-
public static final HBaseClassTestRule CLASS_RULE =
47-
HBaseClassTestRule.forClass(TestMetricsTableRequests.class);
48-
4944
@Test
5045
public void testMetricsTableLatencies() {
5146
TableName tn1 = TableName.valueOf("table1");
5247
TableName tn2 = TableName.valueOf("table2");
5348
MetricsTableRequests requests1 = new MetricsTableRequests(tn1, new Configuration());
5449
MetricsTableRequests requests2 = new MetricsTableRequests(tn2, new Configuration());
55-
assertTrue("'requests' is actually " + requests1.getClass(),
56-
requests1 instanceof MetricsTableRequests);
57-
assertTrue("'requests' is actually " + requests2.getClass(),
58-
requests2 instanceof MetricsTableRequests);
5950

6051
MetricRegistryInfo info1 = requests1.getMetricRegistryInfo();
6152
MetricRegistryInfo info2 = requests2.getMetricRegistryInfo();
@@ -101,8 +92,6 @@ public void testTableQueryMeterSwitch() {
10192
// disable
10293
assertFalse(enableTableQueryMeter);
10394
MetricsTableRequests requests = new MetricsTableRequests(tn1, conf);
104-
assertTrue("'requests' is actually " + requests.getClass(),
105-
requests instanceof MetricsTableRequests);
10695

10796
MetricRegistryInfo info = requests.getMetricRegistryInfo();
10897
Optional<MetricRegistry> registry = MetricRegistries.global().get(info);
@@ -118,15 +107,33 @@ public void testTableQueryMeterSwitch() {
118107
MetricsTableRequests.ENABLE_TABLE_QUERY_METER_METRICS_KEY_DEFAULT);
119108
assertTrue(enableTableQueryMeter);
120109
requests = new MetricsTableRequests(tn1, conf);
121-
assertTrue("'requests' is actually " + requests.getClass(),
122-
requests instanceof MetricsTableRequests);
123110

124111
info = requests.getMetricRegistryInfo();
125112
registry = MetricRegistries.global().get(info);
126113
assertTrue(registry.isPresent());
127114
requests.updateTableReadQueryMeter(500L);
128115
read = registry.get().get("tableReadQueryPerSecond");
129116
assertTrue(read.isPresent());
130-
assertEquals(((DropwizardMeter) read.get()).getCount(), 500);
117+
assertEquals(500, ((DropwizardMeter) read.get()).getCount());
118+
}
119+
120+
@Test
121+
public void testSameRegistryInstanceRefCounting(TestInfo testInfo) {
122+
TableName tn1 = TableName.valueOf(testInfo.getTestMethod().get().getName());
123+
124+
// create registry twice, should return same instance due to ref-counting
125+
MetricsTableRequests requests1 = new MetricsTableRequests(tn1, new Configuration());
126+
MetricsTableRequests requests2 = new MetricsTableRequests(tn1, new Configuration());
127+
128+
MetricRegistry metricRegistry1 = requests1.getMetricRegistry();
129+
MetricRegistry metricRegistry2 = requests2.getMetricRegistry();
130+
// verify ref-counting returns same instance
131+
assertEquals(metricRegistry1, metricRegistry2);
132+
133+
MetricRegistryInfo registryInfo = metricRegistry1.getMetricRegistryInfo();
134+
MetricRegistries.global().remove(registryInfo);
135+
assertTrue(MetricRegistries.global().get(registryInfo).isPresent());
136+
MetricRegistries.global().remove(registryInfo);
137+
assertFalse(MetricRegistries.global().get(registryInfo).isPresent());
131138
}
132139
}

0 commit comments

Comments
 (0)