Skip to content

HBASE-23245 : MutableHistogram constructor changes and provide Histog… #787

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.

Sign up for GitHub

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

Merged
guangxuCheng merged 1 commit into from
Nov 11, 2019
Merged

HBASE-23245 : MutableHistogram constructor changes and provide Histog… #787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/MutableHistogram.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ public MutableHistogram(MetricsInfo info) {
}

public MutableHistogram(String name, String description) {
this(name, description, Integer.MAX_VALUE << 2);
}

protected MutableHistogram(String name, String description, long maxExpected) {
this.name = StringUtils.capitalize(name);
this.desc = StringUtils.uncapitalize(description);
this.histogram = new HistogramImpl();
Expand Down
6 changes: 1 addition & 5 deletions hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/MutableRangeHistogram.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,7 @@ public MutableRangeHistogram(MetricsInfo info) {
}

public MutableRangeHistogram(String name, String description) {
this(name, description, Integer.MAX_VALUE << 2);
}

public MutableRangeHistogram(String name, String description, long expectedMax) {
super(name, description, expectedMax);
super(name, description);
}

/**
Expand Down
7 changes: 2 additions & 5 deletions hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/MutableSizeHistogram.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
*/
@InterfaceAudience.Private
public class MutableSizeHistogram extends MutableRangeHistogram {

private final static String RANGE_TYPE = "SizeRangeCount";
private final static long[] RANGES = {10,100,1000,10000,100000,1000000,10000000,100000000};

Expand All @@ -34,11 +35,7 @@ public MutableSizeHistogram(MetricsInfo info) {
}

public MutableSizeHistogram(String name, String description) {
this(name, description, RANGES[RANGES.length-2]);
}

public MutableSizeHistogram(String name, String description, long expectedMax) {
super(name, description, expectedMax);
super(name, description);
}

@Override
Expand Down
6 changes: 1 addition & 5 deletions hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/MutableTimeHistogram.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,7 @@ public MutableTimeHistogram(MetricsInfo info) {
}

public MutableTimeHistogram(String name, String description) {
this(name, description, RANGES[RANGES.length - 2]);
}

public MutableTimeHistogram(String name, String description, long expectedMax) {
super(name, description, expectedMax);
super(name, description);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion hbase-metrics/src/main/java/org/apache/hadoop/hbase/metrics/impl/HistogramImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public class HistogramImpl implements Histogram {
private final CounterImpl counter;

public HistogramImpl() {
this(Integer.MAX_VALUE << 2);
this((long) Integer.MAX_VALUE << 2);
Copy link
Contributor

@apurtell apurtell Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, this is the change that required the test changes

}

public HistogramImpl(long maxExpected) {
Expand Down
36 changes: 27 additions & 9 deletions hbase-metrics/src/test/java/org/apache/hadoop/hbase/metrics/impl/TestHistogramImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,19 @@ public void testSnapshot() {
Snapshot snapshot = histogram.snapshot();

assertEquals(100, snapshot.getCount());
assertEquals(50, snapshot.getMedian());
assertEquals(49, snapshot.getMedian());
Copy link
Contributor

@xcangCRM xcangCRM Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you help me understanding why this became 49?

Copy link
Contributor Author

@virajjasani virajjasani Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getMin()=0, getMax()=99. Hence, ideally getMedian() should be 49.
All these values are changed after fixing: https://github.com/apache/hbase/pull/787/files#diff-ac08d4adf915ba5ccb72db551e2fef34R39 (which was incorrect before because without long conversion, the value is numeric overload and results -4)

assertEquals(49, snapshot.getMean());
assertEquals(0, snapshot.getMin());
assertEquals(99, snapshot.getMax());
assertEquals(25, snapshot.get25thPercentile());
assertEquals(75, snapshot.get75thPercentile());
assertEquals(90, snapshot.get90thPercentile());
assertEquals(95, snapshot.get95thPercentile());
assertEquals(98, snapshot.get98thPercentile());
assertEquals(99, snapshot.get99thPercentile());
assertEquals(99, snapshot.get999thPercentile());
assertEquals(24, snapshot.get25thPercentile());
assertEquals(74, snapshot.get75thPercentile());
assertEquals(89, snapshot.get90thPercentile());
assertEquals(94, snapshot.get95thPercentile());
assertEquals(97, snapshot.get98thPercentile());
assertEquals(98, snapshot.get99thPercentile());
assertEquals(98, snapshot.get999thPercentile());

assertEquals(51, snapshot.getCountAtOrBelow(50));
assertEquals(100, snapshot.getCountAtOrBelow(50));

// check that histogram is reset.
assertEquals(100, histogram.getCount()); // count does not reset
Expand All @@ -98,5 +98,23 @@ public void testSnapshot() {
assertEquals(198, snapshot.get98thPercentile());
assertEquals(199, snapshot.get99thPercentile());
assertEquals(199, snapshot.get999thPercentile());

IntStream.range(500, 1000).forEach(histogram::update);

snapshot = histogram.snapshot();

assertEquals(500, snapshot.getCount());
assertEquals(749, snapshot.getMedian());
assertEquals(749, snapshot.getMean());
assertEquals(500, snapshot.getMin());
assertEquals(999, snapshot.getMax());
assertEquals(624, snapshot.get25thPercentile());
assertEquals(874, snapshot.get75thPercentile());
assertEquals(949, snapshot.get90thPercentile());
assertEquals(974, snapshot.get95thPercentile());
assertEquals(989, snapshot.get98thPercentile());
assertEquals(994, snapshot.get99thPercentile());
assertEquals(998, snapshot.get999thPercentile());

}
}