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