Hey Luke, Actually the addMetric still throws IllegalArgumentException. The new method that is being introduced as part of the KIP doesn't throw any exceptions. Also, the KIP was an afterthought, which came up during the PR review of this: https://github.com/apache/kafka/pull/12121
Let me know if that's ok. Thanks! Sagar. On Tue, May 31, 2022 at 8:40 AM Luke Chen <show...@gmail.com> wrote: > Hi Sagar, > > Thanks for the KIP. > In the `Compatibility` section, you mentioned: > > *The changes are backward compatible and needs no deprecation/migration.* > > But as you said, we won't throw `IllegalArgumentException` in `addMetric` > method. Isn't that the compatibility issue that should be put in > compatibility section? > I'm thinking, if we don't want to break the backward compatibility, could > we create another registerMetric method for the new added > getMetricOrElseCreate method? > No matter what, we should make the compatibility section clear. > > Thank you. > Luke > > On Wed, May 25, 2022 at 12:56 AM Bruno Cadonna <cado...@apache.org> wrote: > > > Hi Sagar, > > > > Thank you for the updates! > > > > 2. Great if we just need one overload! > > > > Best, > > Bruno > > > > On 24.05.22 18:30, Sagar wrote: > > > Hi Bruno, > > > > > > Thanks for your comments. > > > > > > 1) I updated the header in the public interfaces section. > > > 2) Well, the overloads were there initially but then it was suggested > to > > > remove them during PR review: > > > https://github.com/apache/kafka/pull/12121#discussion_r875156714 > > > 3) I updated the KIP with the function name you suggested. > > > > > > Thanks! > > > Sagar. > > > > > > > > > On Tue, May 24, 2022 at 9:34 PM Bruno Cadonna <cado...@apache.org> > > wrote: > > > > > >> Sagar, Thanks for the KIP! > > >> > > >> > > >> 1. Could you please add the class to which you want to add the method > in > > >> section "Public Interfaces"? Ideally, in the header of the code part > > >> that contains "prefixScan" currently. > > >> > > >> 2. Do you also plan to add overloads for metricOrElseCreate() similar > as > > >> for addMetric()? > > >> > > >> 3. I would propose to call the method getMetricOrElseCreate(). > > >> > > >> Best, > > >> Bruno > > >> > > >> > > >> On 24.05.22 13:47, Sagar wrote: > > >>> Thanks Guozhang. I made the edit. Let me open a VOTE thread. > > >>> > > >>> Thanks! > > >>> Sagar. > > >>> > > >>> On Mon, May 23, 2022 at 10:19 PM Guozhang Wang <wangg...@gmail.com> > > >> wrote: > > >>> > > >>>> Thanks Sagar, > > >>>> > > >>>> The KIP looks good to me overall, I just have a minor comment on the > > >>>> motivation of it: "The Metrics registry is often used by > concurrent > > >>>> threads" -> maybe we can be more clearer, by saying that "Concurrent > > >> thread > > >>>> may try to access the Metrics registry to create the same, > > >> instance-level > > >>>> metrics". > > >>>> > > >>>> Otherwise, +1 from me. Since this is a very straight-forward KIP I > > >> think we > > >>>> can move on to the VOTE phase directly. > > >>>> > > >>>> > > >>>> Guozhang > > >>>> > > >>>> On Mon, May 23, 2022 at 5:46 AM Sagar <sagarmeansoc...@gmail.com> > > >> wrote: > > >>>> > > >>>>> Hi All, > > >>>>> > > >>>>> I would like to start a discussion thread on the following KIP: > > >>>>> > > >>>>> > > >>>>> > > >>>> > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-843%3A+Adding+metricOrElseCreate+method+to+Metrics > > >>>>> > > >>>>> PS: Note that I sent another email without the KIP number in the > > >>>>> subject line. Plz use this email for discussion and ignore the > other > > >> one. > > >>>>> Sorry about the spamming. > > >>>>> > > >>>>> Thanks! > > >>>>> Sagar. > > >>>>> > > >>>> > > >>>> > > >>>> -- > > >>>> -- Guozhang > > >>>> > > >>> > > >> > > > > > >
