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

[zackdever]

this is consistent with the Max stat implementation.

[auradkar]

Seems correct. I think the default value is used to create samples. In this case, the Min function would have reported an incorrect value right? Can you write a test case to prove correctness now?

[zackdever]

Yes I think it would have. I'll see if there's a relevant max test to keep things similar, or if not I'll write one for both.

[zackdever]

I updated testOldDataHasNoEffect to include Min, along with a couple other SampledStats. The updated test fails if ran against trunk. I can make a new test if desired, but the flow would be the exact same - the only way initialValue is ever exposed is by measuring a SampledStat with an old Sample that has been reset. If you just measure a new SampledStat without ever calling record, a Sample is never created and initialValue never comes into play. That might be an argument for using initialValue as the default starting value in the implementation of combine, but that seemed like a more substantial (but functionally equivalent) change.

[dpkp]

+1 !

[auradkar]

nit: i'd prefer a new testcase. this doesn't really have anything to do with 'testOldDataHasNoEffect'

[auradkar]

+1. minor

[zackdever]

@auradkar I updated the test, let me know what you think. I think the Jenkins failures are unrelated.

[zackdever]

Jenkins failures were unrelated - rebased master and all is passing now.

[gwenshap]

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.

[zackdever]

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).

[gwenshap]

fair enough. LGTM.