HBASE-28123 TableRequests metrics broken after altering table#7855
Open
liuxiaocs7 wants to merge 3 commits intoapache:masterfrom
Open
HBASE-28123 TableRequests metrics broken after altering table#7855liuxiaocs7 wants to merge 3 commits intoapache:masterfrom
liuxiaocs7 wants to merge 3 commits intoapache:masterfrom
Conversation
Member
liuxiaocs7
commented
Mar 5, 2026
liuxiaocs7
commented
- see: HBASE-28123
liuxiaocs7
commented
Mar 5, 2026
Comment on lines
65
to
+76
| 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)); |
Member
Author
There was a problem hiding this comment.
this is the fix, we should always use the newest metricRegistry here
Member
Author
|
Hi, @Apache9, what do you think of this change for this bug, more details see HBASE-28123, Thanks! |
Member
Author
|
Now the CI pass all! |
There was a problem hiding this comment.
Pull request overview
Adds regression coverage and fixes for TableRequests metrics behavior around table lifecycle events (alter/reopen, drop), addressing HBASE-28123 where metrics could become stale/broken after an alter.
Changes:
- Fix
GlobalMetricRegistriesAdapterto snapshot metrics from the currentMetricRegistry(lookup byMetricRegistryInfo) instead of holding a stale registry instance. - Rename/clean up
MetricsTableRequestsquery-meter flag + API, and remove unused no-arg meter update methods (plus corresponding unused RS wrapper). - Add/modernize JUnit5 tests, including a new mini-cluster integration test for metrics re-register/unregister behavior and a ref-counting test.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsTableRequestsRegister.java | New mini-cluster regression tests for metrics (re)registration after alter and unregistration after drop. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsTableRequests.java | Move to correct package/JUnit5; add ref-counting test and minor assertion cleanups. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsUserAggregate.java | Update mocks for renamed MetricsTableRequests query-meter enable method. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegionServer.java | Update mocks for renamed MetricsTableRequests query-meter enable method. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/metrics/MetricsTableRequests.java | Rename query-meter enable flag/getter; remove unused no-arg meter methods; restrict registry getters to tests. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServer.java | Remove unused no-arg updateWriteQueryMeter wrapper. |
| hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/metrics/impl/GlobalMetricRegistriesAdapter.java | Ensure metrics2 adapter snapshots from the latest registry instance to survive registry replacement on alter/reopen. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsTableRequests.java:49
- This test class creates MetricRegistries.global() entries (e.g., for "table1"/"table2") but never clears/removes them, so state can leak across tests/classes and make assertions order-dependent/flaky. Add an
@AfterEachthat calls MetricRegistries.global().clear() (or explicitly remove the registries you create) and consider deriving table names from TestInfo to avoid collisions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsTableRequests.java:137
- MetricRegistries.remove returns true only when the registry is actually removed (refCount reaches 0). Consider asserting the return values here (first remove should be false, second should be true) so the test validates ref-count semantics directly, not just presence/absence.
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsTableRequests.java:128 - This class uses MetricRegistries.global() but does not clear it between tests. Since MetricRegistries is global and ref-counted, leftover registries can leak into later tests (and other classes) and make assertions order-dependent. Consider adding an
@AfterEachthat calls MetricRegistries.global().clear() (consistent with TestMetricsRegionServer.java:72-76 and TestMetricsThrottleExceptions.java:53-57).
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsTableRequests.java:131 - The intent here is to verify ref-counting returns the same MetricRegistry instance, but assertEquals would also pass if MetricRegistry ever implements value-based equality. Prefer asserting identity (assertSame) for metricRegistry1/metricRegistry2 to make the test strictly validate instance reuse.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.