Hey everybody, Just wanted to say that I plan on starting a voting thread tomorrow if there aren't any further comments.
Thanks for the discussion so far! On Wed, Oct 24, 2018 at 11:27 AM Stanislav Kozlovski <stanis...@confluent.io> wrote: > Hi Kevin, > > Thanks for providing context. > > I've edited the KIP to use `NaN` for all three metric types. > Here is the updated link: KIP-386: Standardize on Min/Avg/Max metrics' > default value > <https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95652345> > > On Tue, Oct 23, 2018 at 7:30 PM Kevin Lu <lu.ke...@berkeley.edu> wrote: > >> Hi Stanislav, >> >> Thanks for the KIP! >> >> Standardizing this would be extremely helpful. We have been publishing >> client-side metrics to a time-series database using metrics reporter since >> 0.8, and we had to do explicit checks like "!Double.isNaN(metricValue)", >> "metricValue != Double.NEGATIVE_INFINITY", and such to skip these values. >> >> I like John's suggestion for using NaN for the same reason that -INF >> and +INF are still technically valid values as they are just the extreme >> bounds. >> >> Regards, >> Kevin >> >> On Tue, Oct 23, 2018 at 3:46 AM Stanislav Kozlovski < >> stanis...@confluent.io> >> wrote: >> >> > Hey John, >> > >> > I think NaN would be the better option semantically. If we were to use >> > that, maybe it makes sense to change `Avg()`'s default value from 0.0 to >> > NaN as well. >> > >> > I am a bit more concerned with backwards compatibility if we were to use >> > NaN since we would change three types of metrics. There were only three >> > metrics that use `Min` but `Avg` and `Max` are used everywhere. >> > I guess it boils down to how likely it is that such a change can break >> > users' tools and if we're okay with it. My assumption is that it won't - >> > most tools should be mature enough to handle these values. >> > >> > Thanks for the suggestion! I will wait out a bit more for other people >> to >> > share their thoughts and update the KIP with `NaN` >> > >> > >> > On Tue, Oct 23, 2018 at 6:12 AM John Roesler <j...@confluent.io> wrote: >> > >> > > Hi Stanislav, >> > > Thanks for this KIP. I coincidentally just noticed these strange >> initial >> > > values while doing some performance measurements. >> > > >> > > Since the metric type is a double, could we consider NaN instead? It >> > seems >> > > like +Inf is somewhat arbitrary for Max, so it might as well be an >> > > arbitrary value that actually means "this is not a number". >> > > >> > > Consider that +- Infinity is technically "in range" for max and min, >> > while >> > > NaN is not. NaN is "in range" for an average or a rate, but in those >> > cases >> > > it would mean that the result is over 0 samples or 0 time, >> respectively. >> > I >> > > think this only happens when nothing has been recorded, so it would >> still >> > > be sound for the situation you're attempting to address. >> > > >> > > Just to throw it out there, `null` is technically also (maybe moreso) >> a >> > > sound choice, but I'd be concerned about causing a bunch of null >> > > dereference errors. >> > > >> > > Thanks again, >> > > -John >> > > >> > > On Mon, Oct 22, 2018 at 2:27 PM Stanislav Kozlovski < >> > > stanis...@confluent.io> >> > > wrote: >> > > >> > > > Hi Suman, >> > > > >> > > > Thanks for taking a look at this. Yeah, it's very minor but it >> changes >> > > the >> > > > public API (even if this is a very slight change) and as far as I >> know >> > > this >> > > > warrants some discussion >> > > > >> > > > On Mon, Oct 22, 2018 at 9:28 PM Suman B N <sumannew...@gmail.com> >> > wrote: >> > > > >> > > > > Looks good to me. Maintains uniformity. +1 from me. >> > > > > But its more of a bug rather than improvement proposal. Let's see >> > what >> > > > > contributors got to say. >> > > > > >> > > > > On Mon, Oct 22, 2018 at 7:23 PM Stanislav Kozlovski < >> > > > > stanis...@confluent.io> >> > > > > wrote: >> > > > > >> > > > > > Hey everybody, >> > > > > > >> > > > > > I've opened up a very short KIP to make the Max and Min metrics' >> > > > default >> > > > > > values consistent with each other. >> > > > > > >> > > > > > KIP: >> > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-386%3A+Make+Min+metrics%27+default+value+consistent+with+Max+metrics >> > > > > > JIRA: https://issues.apache.org/jira/browse/KAFKA-7528 >> > > > > > >> > > > > > This is hopefully a very straightforward change. Please provide >> > > > feedback. >> > > > > > Thanks >> > > > > > >> > > > > > -- >> > > > > > Best, >> > > > > > Stanislav >> > > > > > >> > > > > >> > > > > >> > > > > -- >> > > > > *Suman* >> > > > > *OlaCabs* >> > > > > >> > > > >> > > > >> > > > -- >> > > > Best, >> > > > Stanislav >> > > > >> > > >> > >> > >> > -- >> > Best, >> > Stanislav >> > >> > > > -- > Best, > Stanislav > -- Best, Stanislav