mnpoonia commented on code in PR #8389:
URL: https://github.com/apache/hbase/pull/8389#discussion_r3781475516
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerWrapperStub.java:
##########
@@ -87,6 +87,16 @@ public long getStoreFileSize() {
return 1900;
}
+ @Override
Review Comment:
This verifies source export from a fixed wrapper stub, but it does not
exercise `MetricsRegionServerWrapperImpl` aggregation or ratio calculation.
Could we extend `TestMetricsRegionServerAggregate` to assert the summed
compressed/uncompressed sizes and `uncompressed / compressed` ratio, including
the empty aggregate case? Using stores with different individual ratios would
also verify this remains a weighted aggregate rather than an average of ratios.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerWrapperImpl.java:
##########
@@ -533,6 +533,20 @@ public long getStoreFileSize() {
return aggregate.storeFileSize;
}
+ @Override
+ public long getStoreFileUncompressedSize() {
+ return aggregate.storeFileUncompressedSize;
+ }
+
+ @Override
+ public double getStoreFileCompressionRatio() {
+ long uncompressed = aggregate.storeFileUncompressedSize;
Review Comment:
`aggregate` is a volatile snapshot and can be replaced between these two
reads. That can calculate the ratio from different refreshes, including
`nonzero / 0` when regions are removed. Could we capture one
`RegionMetricAggregate current = aggregate`, read both values from `current`,
and guard the compressed denominator before dividing?
```
RegionMetricAggregate current = aggregate;
if (current.storeFileSize <= 0) {
return 0.0;
}
return (double) current.storeFileUncompressedSize / current.storeFileSize;
```
--
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]
