HBASE-27224 HFile tool statistic sampling produces misleading results #4638
Conversation
This file contains 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
|
🎊 +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. |
cbaenziger
reviewed
Jul 26, 2022
| @@ -640,49 +722,39 @@ public String toString() { | |||
| if (prevCell == null) return "no data available for statistics"; | |||
|
|
|||
| // Dump the metrics to the output stream | |||
| simpleReporter.stop(); | |||
There was a problem hiding this comment.
This was unexpected at first to me as I am not very CodaHale aware but I questioned why remove the simpleReporter.stop()? (It does seem odd to stop the reporter in a toString() implementation but I think this was a hack to clean the reporter up when emitting its metrics?)
As I understand now simpleReporter is no longer based on a CodaHale metric class with any thread or other state to clean-up though so no need for a stop() method?
There was a problem hiding this comment.
Yea, so this previously had SimpleReporter extend ScheduledReporter. In my opinion this was overkill. The benefits of ScheduledReporter are:
- it has a thread which can print the statistics periodically over time. You need to call reporter.start() for that.
- It can do various filters on the metrics, which is mostly useful when you have larger shared MetricsRegistry and maybe only care about certain metrics.
- It can simplify the collection of multiple different metric types (counters, gauges, etc). Including some extra pre-processing that can be applied.
We weren't using any of these features, so I think it was unnecessary to use ScheduledReporter. I would have been fine to keep it in place just to simplify this PR, but there were issues with ScheduledReporter.
Our primary goal here was to add a global min/max for each of the metrics collected. Since ScheduledReporter is based around processing codahale metrics, it's hard to supplement those metrics with additional metadata. I think the "codahale-native" way for me to do this would have been to add a new Gauge for all of the existing Histograms. Then, in the report method below we'd get those gauges alongside the histograms. The annoying thing with that is we'd have to manually match up the names somehow, and the names have to be unique. So maybe I'd name the histogram Key length and then the gauge Key length - max and have to do some string munging to link the 2 together.
That felt pretty hacky and all for the purposes of keeping a thing we don't need/use anyway. I also tried some other hacks to keep it, liking having a static map which kept them together, but this felt similarly hacky. So that's why I ripped out the ScheduledReporter.
With ScheduledReporter, the report method is triggered when you call stop (as well as periodically by the reporting thread if you had called start(), which we don't). So you're correct, that's why we used to have to call stop(). Now that we don't rely on ScheduledReporter, we don't need to call that at all.
| @@ -760,6 +830,37 @@ private void printHistogram(Histogram histogram) { | |||
| output.printf(locale, " 99%% <= %2.2f%n", snapshot.get99thPercentile()); | |||
| output.printf(locale, " 99.9%% <= %2.2f%n", snapshot.get999thPercentile()); | |||
| output.printf(locale, " count = %d%n", histogram.getCount()); | |||
|
|
|||
There was a problem hiding this comment.
The below block is really unobvious to me how it is (presumably) printing mins and maxes but it may just be late for me. I'll take another look tomorrow. Perhaps a comment of what the process being implemented is could be of assistance?
There was a problem hiding this comment.
The min/max change is handled above on lines 822/823. The below logic prints the counts-by-range, i.e. the last 4 lines here:
Key length:
min = 43
max = 137
mean = 104.97
stddev = 25.93
median = 105.00
75% <= 137.00
95% <= 137.00
98% <= 137.00
99% <= 137.00
99.9% <= 137.00
count = 852277
0 <= 10
785 <= 50
404643 <= 100
446849 <= 500
I could add a comment here just to describe what's happening. It's similar to how we handle range counts in JMX
There was a problem hiding this comment.
@cbaenziger just pushed a comment, let me know if it clears anything up
There was a problem hiding this comment.
Thanks! The code and review comments makes it more clear what is intended. I think the output will need some explanation of what is being displayed after the count.
Right now, it looks like it's on lines 864-866 printing:
<count> <= <range>
While all the other lines print:
<descriptor> <= <value from data>
Could the range count be reversed to match the layout of the other lines?
<range> <= <count>
Lastly, this may be a question/solution chasing a problem, but will this show a bi-modal set of ranges clearly (e.g. if I have keys of 50 bytes and keys of 5,000 bytes only) or will the elided bounding ranges be needed to point that out? Or is that what line 845 is doing already?
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
cbaenziger
reviewed
Aug 2, 2022
| @@ -760,6 +830,37 @@ private void printHistogram(Histogram histogram) { | |||
| output.printf(locale, " 99%% <= %2.2f%n", snapshot.get99thPercentile()); | |||
| output.printf(locale, " 99.9%% <= %2.2f%n", snapshot.get999thPercentile()); | |||
| output.printf(locale, " count = %d%n", histogram.getCount()); | |||
|
|
|||
There was a problem hiding this comment.
Thanks! The code and review comments makes it more clear what is intended. I think the output will need some explanation of what is being displayed after the count.
Right now, it looks like it's on lines 864-866 printing:
<count> <= <range>
While all the other lines print:
<descriptor> <= <value from data>
Could the range count be reversed to match the layout of the other lines?
<range> <= <count>
Lastly, this may be a question/solution chasing a problem, but will this show a bi-modal set of ranges clearly (e.g. if I have keys of 50 bytes and keys of 5,000 bytes only) or will the elided bounding ranges be needed to point that out? Or is that what line 845 is doing already?
|
@cbaenziger thanks for the input. I made changes per your request. Here's the updated format: Worth noting that the new To your last question:
|
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Sep 2, 2022
bbeaudreault
added a commit
that referenced
this pull request
Sep 6, 2022
…#4638) Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Clay Baenziger <cwb@clayb.net>
bbeaudreault
added a commit
that referenced
this pull request
Sep 6, 2022
…#4638) Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Clay Baenziger <cwb@clayb.net>
bbeaudreault
added a commit
to HubSpot/hbase
that referenced
this pull request
Sep 23, 2022
…apache#4638) Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Clay Baenziger <cwb@clayb.net>
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…apache#4638) Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Clay Baenziger <cwb@clayb.net> (cherry picked from commit 6f6857b) Change-Id: Ibf797084adc46339b1c8d856f2359914381c277e
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.
Wraps the existing codahale Histogram in a KeyValueStats object. This object tracks the global min and max sizes for the statistic, since Histogram's is subject to sampling. Additionally adds a new
-dargument which enables printing of detailed size range counts. This can be useful for further visualizing the distribution of sizes.Below is an example form a real HFile. Prior to this patch, the max value shown was only 25k due to the sampling done by Histogram. Additionally, the new buckets (printed when
-dis passed) make it easy to see that there are actually a few doze quite large rows in this file.