Skip to content

MINOR: change initial value of Min stat to Double.MAX_VALUE (not MIN) #1143

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

Closed
zackdever wants to merge 2 commits into from
Closed

MINOR: change initial value of Min stat to Double.MAX_VALUE (not MIN) #1143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e4b760
change initial value of Min stat to Double.MAX_VALUE (not MIN)
zackdever Mar 25, 2016
f3c7403
create new testSampledStatInitialValue
zackdever Apr 14, 2016
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
8 changes: 4 additions & 4 deletions clients/src/main/java/org/apache/kafka/common/metrics/stats/Min.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
public class Min extends SampledStat {

public Min() {
super(Double.MIN_VALUE);
super(Double.MAX_VALUE);
Copy link
Copy Markdown
Contributor

@gwenshap gwenshap Apr 29, 2016

Choose a reason for hiding this comment

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

Nice catch (and sorry for long delay in review).
I saw we are using NEGATIVE_INFINITY for Max() default. Any reason not to use INFINITY here? It seems more consistent.

Copy link
Copy Markdown
Contributor Author

@zackdever zackdever Apr 29, 2016

Choose a reason for hiding this comment

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

No worries. I noticed that too, and I think it would be more consistent to use INFINITY, but the convention seems to be that the combine implementation uses the same value for the de facto default (even though it doesn't make use of initialValue). Min() is already using Double.MAX_VALUE in combine, so also changing that felt like a bigger change (and possibly unwelcome if users were already handling Double.MAX_VALUE in their metrics tooling).

Copy link
Copy Markdown
Contributor

@gwenshap gwenshap Apr 29, 2016

Choose a reason for hiding this comment

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

fair enough. LGTM.

}

@Override
Expand All @@ -32,10 +32,10 @@ protected void update(Sample sample, MetricConfig config, double value, long now

@Override
public double combine(List<Sample> samples, MetricConfig config, long now) {
double max = Double.MAX_VALUE;
double min = Double.MAX_VALUE;
for (int i = 0; i < samples.size(); i++)
max = Math.min(max, samples.get(i).value);
return max;
min = Math.min(min, samples.get(i).value);
return min;
}

}
32 changes: 32 additions & 0 deletions clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,38 @@ public void testOldDataHasNoEffect() {
assertEquals(Double.NEGATIVE_INFINITY, max.measure(config, time.milliseconds()), EPS);
}

@Test
public void testSampledStatInitialValue() {
// initialValue from each SampledStat is set as the initialValue on its Sample.
// The only way to test the initialValue is to infer it by having a SampledStat
// with expired Stats, because their values are reset to the initial values.
// Most implementations of combine on SampledStat end up returning the default
// value, so we can use this. This doesn't work for Percentiles though.
// This test looks a lot like testOldDataHasNoEffect because it's the same
// flow that leads to this state.
Max max = new Max();
Min min = new Min();
Avg avg = new Avg();
Count count = new Count();
Rate.SampledTotal sampledTotal = new Rate.SampledTotal();

long windowMs = 100;
int samples = 2;
MetricConfig config = new MetricConfig().timeWindow(windowMs, TimeUnit.MILLISECONDS).samples(samples);
max.record(config, 50, time.milliseconds());
min.record(config, 50, time.milliseconds());
avg.record(config, 50, time.milliseconds());
count.record(config, 50, time.milliseconds());
sampledTotal.record(config, 50, time.milliseconds());
time.sleep(samples * windowMs);

assertEquals(Double.NEGATIVE_INFINITY, max.measure(config, time.milliseconds()), EPS);
assertEquals(Double.MAX_VALUE, min.measure(config, time.milliseconds()), EPS);
assertEquals(0.0, avg.measure(config, time.milliseconds()), EPS);
assertEquals(0, count.measure(config, time.milliseconds()), EPS);
assertEquals(0.0, sampledTotal.measure(config, time.milliseconds()), EPS);
}

@Test(expected = IllegalArgumentException.class)
public void testDuplicateMetricName() {
metrics.sensor("test").add(metrics.metricName("test", "grp1"), new Avg());
Expand Down