Copilot commented on code in PR #6486:
URL: https://github.com/apache/hbase/pull/6486#discussion_r2840113722
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsTableWrapperAggregateImpl.java:
##########
@@ -76,8 +76,10 @@ public void run() {
mt.storeFileCount += store.getStorefilesCount();
mt.maxStoreFileCount = Math.max(mt.maxStoreFileCount,
store.getStorefilesCount());
- mt.memstoreSize += (store.getMemStoreSize().getDataSize()
- + store.getMemStoreSize().getHeapSize() +
store.getMemStoreSize().getOffHeapSize());
+ final MemStoreSize memstoreSize = store.getMemStoreSize();
+ mt.memstoreSize += memstoreSize.getDataSize();
+ mt.memstoreHeapSize += memstoreSize.getHeapSize();
+ mt.memstoreOffHeapSize += memstoreSize.getOffHeapSize();
Review Comment:
This change corrects the table-level memstoreSize calculation (now using
MemStoreSize#getDataSize and exporting heap/off-heap separately), but there is
no direct test covering MetricsTableWrapperAggregateImpl's aggregation
behavior. Consider adding a focused unit/integration test that stubs a
Region/Store MemStoreSize and asserts
getMemStoreSize/getMemStoreHeapSize/getMemStoreOffHeapSize return the expected
aggregated values to prevent regressions.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegion.java:
##########
@@ -62,6 +62,12 @@ public void testRegionWrapperMetrics() {
HELPER.assertGauge(
"namespace_TestNS_table_MetricsRegionWrapperStub_region_DEADBEEF001_metric_memstoreSize",
103,
agg);
+ HELPER.assertGauge(
+
"namespace_TestNS_table_MetricsRegionWrapperStub_region_DEADBEEF001_metric_memstoreHeapSize",
104,
+ agg);
+ HELPER.assertGauge(
+
"namespace_TestNS_table_MetricsRegionWrapperStub_region_DEADBEEF001_metric_memstoreOffHeapSize",
105,
+ agg);
Review Comment:
The newly added assertGauge lines for memstoreHeapSize/memstoreOffHeapSize
appear to exceed the project's Checkstyle LineLength limit (max 100). Please
wrap these assertions across multiple lines similar to the surrounding
assertGauge calls to avoid Checkstyle failures.
```suggestion
"namespace_TestNS_table_MetricsRegionWrapperStub_region_DEADBEEF001_metric_memstoreHeapSize",
104, agg);
HELPER.assertGauge(
"namespace_TestNS_table_MetricsRegionWrapperStub_region_DEADBEEF001_metric_memstoreOffHeapSize",
105, agg);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
