Hi Sagar, Yes, I think I misunderstood your KIP. I have no other comments, then.
Thank you. Luke On Tue, May 31, 2022 at 12:46 PM Sagar <sagarmeansoc...@gmail.com> wrote: > 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 > > > >>>> > > > >>> > > > >> > > > > > > > > > >
