kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Roesler <j...@confluent.io>
Subject Re: [DISCUSS] KIP-488: Clean up Sum,Count,Total Metrics
Date Mon, 15 Jul 2019 16:06:58 GMT
Hey Stanislav,

Thanks for the feedback! I do think we should try to keep the scope of
this KIP small. The idea you proposed is orthogonal, though, so if you
wanted to write up a quick KIP concurrently, I'd be happy to review
it.

Regarding deprecation: yes, the metrics are in the public interface,
because they are not inside a package named `internal`. Therefore, we
do need to do deprecations and migrations when considering changes to
them. I wish we didn't, since it could be a cleaner transition without
them, but we shouldn't risk breaking source compatibility.

Thanks,
-John

On Sat, Jul 13, 2019 at 1:30 PM Stanislav Kozlovski
<stanislav@confluent.io> wrote:
>
> Hello,
>
> Thanks for the KIP, I'm happy we're converging on implementations.
>
> I was wondering whether we need to deprecate the old metrics - are those
> classes public interfaces? I see we don't build a JavaDoc for them (
> https://kafka.apache.org/22/javadoc/allclasses-noframe.html) and, as far as
> I know, that means we don't necessarily need to go with the deprecation
> route.
>
> I think we also have the Measurable and Gauge classes which seem to do the
> same thing. From the names, I intuitively thought that Measurable indicates
> something that might require some amount of processing to get the value of
> and Gauge to be something that is instantly read. Reading their usages,
> though, I see that is not the case. Measurable is inherited by
> MeasurableStat which itself is inherited by most of the classes you
> mentioned in the KIP. Is it worth it converging on those as well (perhaps
> deprecating/removing Measurable and opting for Gauge) or do we prefer to
> keep the scope of this KIP small?
>
> Thanks,
> Stanislav
>
> On Fri, Jul 12, 2019 at 7:42 PM John Roesler <john@confluent.io> wrote:
>
> > Hey, thanks Matthias and Bruno,
> >
> > I agree, "Cumulative" is a mouthful. "TotalX" sounds fine to me.
> >
> > Also, yes, I would have liked to not have any modifier for
> > "non-sampled", but there is a name conflict with Sum.
> >
> > I'll update the KIP to reflect "TotalX" and then start the vote thread.
> >
> > Thanks again,
> > -John
> >
> > On Fri, Jul 12, 2019 at 11:27 AM Bruno Cadonna <bruno@confluent.io> wrote:
> > >
> > > OK, makes sense. Then, I am in favour of TotalCount and TotalSum.
> > >
> > > Best,
> > > Bruno
> > >
> > > On Fri, Jul 12, 2019 at 12:57 AM Matthias J. Sax <matthias@confluent.io>
> > wrote:
> > > >
> > > > `Sum` is an existing name, for the "sampled sum" metric, that gets
> > > > deprecated. Hence, we cannot use it.
> > > >
> > > > If we cannot use `Sum` and use `TotalSum`, we should also not use
> > > > `Count` but `TotalCount` for consistency.
> > > >
> > > >
> > > > -Matthias
> > > >
> > > >
> > > >
> > > > On 7/11/19 12:58 PM, Bruno Cadonna wrote:
> > > > > Hi John,
> > > > >
> > > > > Thank you for the KIP.
> > > > >
> > > > > LGTM
> > > > >
> > > > > I also do not like CumulativeSum/Count so much. I propose to just
> > call
> > > > > it Sum and Count.
> > > > >
> > > > > I understand that you want to unequivocally distinguish the two
> > metric
> > > > > functions by their names, but I have the feeling the names become
> > > > > artificially complex. The exact semantics can also be documented
in
> > > > > the javadocs, which btw could also be improved in those classes.
> > > > >
> > > > > Best,
> > > > > Bruno
> > > > >
> > > > >
> > > > >
> > > > > On Thu, Jul 11, 2019 at 8:25 PM Matthias J. Sax <
> > matthias@confluent.io> wrote:
> > > > >>
> > > > >> Thanks for the KIP. Overall LGTM.
> > > > >>
> > > > >> The only though I have is, if we may want to use `TotalSum` and
> > > > >> `TotalCount` instead of `CumulativeSum/Count` as names?
> > > > >>
> > > > >>
> > > > >> -Matthias
> > > > >>
> > > > >>
> > > > >> On 7/11/19 9:31 AM, John Roesler wrote:
> > > > >>> Hi Kafka devs,
> > > > >>>
> > > > >>> I'd like to propose KIP-488 as a minor cleanup of some of
our
> > metric
> > > > >>> implementations.
> > > > >>>
> > > > >>> KIP-488: https://cwiki.apache.org/confluence/x/kkAyBw
> > > > >>>
> > > > >>> Over time, iterative updates to these metrics has resulted
in a
> > pretty
> > > > >>> confusing little collection of classes, and I've personally
been
> > > > >>> involved in three separate moderately time-consuming iterations
of
> > me
> > > > >>> or someone else trying to work out which metrics are available,
and
> > > > >>> which ones are desired for a given use case. One of these
was
> > actually
> > > > >>> a long-running bug in Kafka Streams' metrics, so not only
has this
> > > > >>> confusion been a time sink, but it has also led to bugs.
> > > > >>>
> > > > >>> I'm hoping this change won't be too controversial.
> > > > >>>
> > > > >>> Thanks,
> > > > >>> -John
> > > > >>>
> > > > >>
> > > >
> >

Mime
View raw message