-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-27532 Add block bytes scanned metrics #5067
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
ca0171d
to
44ccb81
Compare
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@Apache9 any chance you have some time to review this metrics PR? mostly boilerplate. There are some spotbugs/checkstyle warnings, but they all are unrelated to my changes. For example, checkstyle doesn't like the code style of ServerSideScanMetrics where we expose public instance variables, but I can't change that here. And spotbugs doesn't like how MetricsUserSourceImpl has some weird synchronization for the LossyCounting stuff. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@sunhelly any chance you can review this? Just metrics boilerplate. Thanks |
@@ -116,7 +119,7 @@ public MetricsUserSourceImpl(String user, MetricsUserAggregateSourceImpl agg) { | |||
this.user = user; | |||
this.registry = agg.getMetricsRegistry(); | |||
|
|||
this.userNamePrefix = "user_" + user + "_metric_"; | |||
this.userNamePrefix = "User_" + user + "_metric_"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is ok because all of the existing newTimeHistogram
calls already capitalize the first letter. So these end up in JMX like User_foo_metric_...
. But newCounter
doesn't do the same capitalization, so the new blockBytesScannedCount ends up user_foo_metric_blockBytesScannedCount
(lowercase u). So this change here just ensures that all the metrics are similarly capitalized.
💔 -1 overall
This message was automatically generated. |
Rename server side counter to blockBytesScanned Add to ScanMetrics as countOfBlockBytesScanned, and incorporate into TestScannerBlockSizeLimits
…er metrics in jmx
99c7790
to
6dc20c7
Compare
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Please fix the spotless issues before committing, thanks. |
Thanks for reviewing @sunhelly! The spotless is clear, but there is checkstyle/spotbugs that are all unrelated. I filed two JIRAs to clean them up, since they are distinct: (checkstyle) https://issues.apache.org/jira/browse/HBASE-27757 For now I just pushed a commit to at least suppress the checkstyle warning. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Signed-off-by: Xiaolin Ha <[email protected]>
countofBlockBytesScanned
to ScanMetrics, and incorporates that into TestScannerBlockSizeLimits assertions.responseBlockBytes
toblockBytesScanned
. This seems more straightforward in the context of a request.blockBytesScannedCount
to Regionserver and TableLatencies metricsThis is mostly boilerplate. Since blockBytesScanned increments globally for a request, we have to treat it similarly to time when calling the various per-request update methods -- subtract
after - before
for multi's to get the per-action value.