Hi all, I’m closing the voting on this one. We already had 3 binding votes on this from Guozhang, Bruno and John.
Thanks everyone! Sagar. On Sun, 5 Jun 2022 at 1:53 PM, Sagar <sagarmeansoc...@gmail.com> wrote: > Hi All, > > I am planning to use the name of the new function as addMetricIfAbsent. > Let me know if there are any other suggestions/objections to this. > > Also, @Ismael, I agree with Guozhang about not deprecating the existing > functions. Plz let me know if you have any reservations about that. > > If we all agree to these 2 points, then I would mark the KIP as voted. > > Thanks! > Sagar. > > On Wed, Jun 1, 2022 at 11:47 PM Guozhang Wang <wangg...@gmail.com> wrote: > >> I think `addMetricIfAbsent` is a good function name, and like John said >> people in the Java world are familiar with its return value semantics as >> well. >> >> Regarding deprecating the existing functions, I feel it is not necessary >> just for the function semantics difference between `sensors` and `metrics` >> (Ismael may chime in here if you have other good reasons). >> >> >> Guozhang >> >> On Wed, Jun 1, 2022 at 9:50 AM Sagar <sagarmeansoc...@gmail.com> wrote: >> >> > So, we have multiple options in terms of names, at this point I actually >> > liked John's suggestion to use addMetricIfAbsent or something along >> those >> > lines. >> > >> > Regarding the deprecation of sensor/metric method, I am not sure... >> Would >> > like to know others' thoughts. >> > >> > Thanks! >> > Sagar. >> > >> > On Wed, Jun 1, 2022 at 2:28 AM Guozhang Wang <wangg...@gmail.com> >> wrote: >> > >> > > Hey Ismael, just checking do you mean the `metric` method instead? >> > > >> > > On Tue, May 31, 2022 at 1:45 PM Ismael Juma <ism...@juma.me.uk> >> wrote: >> > > >> > > > Should we deprecate the `sensor` method? One other thing to take >> into >> > > > account is that these methods are meant to be used like a dsl for >> > > > configuring sensors and metrics. So brevity is a plus (but clarity >> is >> > > > critical still). >> > > > >> > > > Ismael >> > > > >> > > > On Tue, May 31, 2022 at 11:09 AM John Roesler <vvcep...@apache.org> >> > > wrote: >> > > > >> > > > > Generally, I agree with Ismael that having a new, weird name will >> > make >> > > it >> > > > > hard to keep them straight. Then again, we need to make them >> > different >> > > to >> > > > > prevent confusion about their semantics. To be clear, I'll be a +1 >> > > > > regardless of how we break this dilemma. >> > > > > >> > > > > One suggestion: We currently have addMetric to add a new metric. >> We >> > can >> > > > > take some inspiration from the Java Map interface and call this >> new >> > > > method >> > > > > `addMetricIfAbsent`. Having the same prefix should help discovery, >> > and >> > > > > following the Map convention should help confusion. >> > > > > >> > > > > Thanks all, >> > > > > -John >> > > > > >> > > > > >> > > > > >> > > > > On Tue, May 31, 2022, at 12:13, Sagar wrote: >> > > > > > Oh yeah there's another metric function which is get-only. I >> think >> > we >> > > > > > should go ahead with getOrCreateMetric. >> > > > > > >> > > > > > Thanks! >> > > > > > Sagar. >> > > > > > >> > > > > > On Tue, May 31, 2022 at 10:02 PM Guozhang Wang < >> wangg...@gmail.com >> > > >> > > > > wrote: >> > > > > > >> > > > > >> I'd prefer the getOrCreateMetric function name, since for the >> > > > existing " >> > > > > >> sensor(String name)" function that only takes a single `String` >> > > > > parameter, >> > > > > >> its semantics is already "get or create". Whereas the existing >> > > > > >> "metric(MetricName)" function's semantics is "get" only. So in >> my >> > > > mind, >> > > > > the >> > > > > >> inconsistent conventions in function signatures already exist >> > today. >> > > > And >> > > > > >> with the other option we would need to educate users that "all >> the >> > > > > `sensor` >> > > > > >> functions are get-or-create, but, please remember that the >> > `metric` >> > > > > >> function with just the metric name is get-only, while other >> > `metric` >> > > > > >> overrides with more parameters are get-or-create", which I >> think >> > is >> > > > even >> > > > > >> more confusing. >> > > > > >> >> > > > > >> >> > > > > >> Guozhang >> > > > > >> >> > > > > >> >> > > > > >> On Mon, May 30, 2022 at 9:51 PM Sagar < >> sagarmeansoc...@gmail.com> >> > > > > wrote: >> > > > > >> >> > > > > >> > Hi Ismael, >> > > > > >> > >> > > > > >> > I guess in that case, we will have to go with the name >> *metric*- >> > > > > similar >> > > > > >> to >> > > > > >> > *sensor* - which David pointed out above because I think >> that's >> > > the >> > > > > >> closest >> > > > > >> > method which either gets or creates a new sensor. Current >> > > addMetric >> > > > in >> > > > > >> the >> > > > > >> > Metrics class throw an IllegalArguementException when the >> metric >> > > > > already >> > > > > >> > exists and that's why I still think getOrCreateMetric still >> > > > signifies >> > > > > the >> > > > > >> > action correctly. Or how about addOrGetMetric or >> getOrAddMetric, >> > > > just >> > > > > >> > replacing create with add to keep it similar to the already >> > > present >> > > > > >> > addMetric method. >> > > > > >> > >> > > > > >> > Thanks! >> > > > > >> > Sagar. >> > > > > >> > >> > > > > >> > On Tue, May 31, 2022 at 1:19 AM Ismael Juma < >> ism...@juma.me.uk> >> > > > > wrote: >> > > > > >> > >> > > > > >> > > I think it's confusing to use two completely different >> naming >> > > > > >> conventions >> > > > > >> > > in the same class. We either stick with the existing >> > convention >> > > or >> > > > > we >> > > > > >> > > create a new one and deprecate old method(s). I am not sure >> > > there >> > > > is >> > > > > >> > enough >> > > > > >> > > value in this case for the latter, but it would be good to >> > hear >> > > > what >> > > > > >> > others >> > > > > >> > > think. >> > > > > >> > > >> > > > > >> > > Ismael >> > > > > >> > > >> > > > > >> > > On Mon, May 30, 2022, 2:08 AM Bruno Cadonna < >> > cado...@apache.org >> > > > >> > > > > >> wrote: >> > > > > >> > > >> > > > > >> > > > Hi, >> > > > > >> > > > >> > > > > >> > > > I would also lean towards getOrCreateMetric() for the >> > reasons >> > > > > pointed >> > > > > >> > > > out by Sagar. But I am fine either way. >> > > > > >> > > > >> > > > > >> > > > Best, >> > > > > >> > > > Bruno >> > > > > >> > > > >> > > > > >> > > > On 30.05.22 10:54, Sagar wrote: >> > > > > >> > > > > Hi Bruno/David, >> > > > > >> > > > > >> > > > > >> > > > > Thanks for the suggestions. I would personally lean >> > towards >> > > > > using >> > > > > >> > > > > getOrCreateMetric as it clearly explains the intent. >> > Having >> > > > said >> > > > > >> > that, >> > > > > >> > > if >> > > > > >> > > > > we want to use just metric(similar to sensor), that >> should >> > > > also >> > > > > be >> > > > > >> > ok. >> > > > > >> > > > Just >> > > > > >> > > > > that I feel getOrCreateMetric is easily understandable. >> > > > > >> > > > > >> > > > > >> > > > > Thanks! >> > > > > >> > > > > Sagar. >> > > > > >> > > > > >> > > > > >> > > > > On Mon, May 30, 2022 at 2:16 PM David Jacot >> > > > > >> > > <dja...@confluent.io.invalid >> > > > > >> > > > > >> > > > > >> > > > > wrote: >> > > > > >> > > > > >> > > > > >> > > > >> Hi all, >> > > > > >> > > > >> >> > > > > >> > > > >> Looking at the current Metrics' API, we have `sensor` >> > which >> > > > > gets >> > > > > >> or >> > > > > >> > > > creates >> > > > > >> > > > >> a sensor. How about using `metric` to follow the same >> > > naming >> > > > > >> > > convention? >> > > > > >> > > > >> >> > > > > >> > > > >> Best, >> > > > > >> > > > >> David >> > > > > >> > > > >> >> > > > > >> > > > >> On Mon, May 30, 2022 at 9:18 AM Bruno Cadonna < >> > > > > cado...@apache.org >> > > > > >> > >> > > > > >> > > > wrote: >> > > > > >> > > > >>> >> > > > > >> > > > >>> Hi Sagar, >> > > > > >> > > > >>> Hi Ismael, >> > > > > >> > > > >>> >> > > > > >> > > > >>> what about getOrCreateMetric()? >> > > > > >> > > > >>> >> > > > > >> > > > >>> Best, >> > > > > >> > > > >>> Bruno >> > > > > >> > > > >>> >> > > > > >> > > > >>> >> > > > > >> > > > >>> On 28.05.22 18:56, Sagar wrote: >> > > > > >> > > > >>>> Hi Ismael, >> > > > > >> > > > >>>> >> > > > > >> > > > >>>> Actually Bruno suggested renaming it to >> > > > getMetricOrElseCreate >> > > > > >> and >> > > > > >> > we >> > > > > >> > > > >>>> decided to go ahead with that one. These were the >> only >> > > > names >> > > > > >> that >> > > > > >> > we >> > > > > >> > > > >>>> considered for the KIP. >> > > > > >> > > > >>>> >> > > > > >> > > > >>>> Thanks! >> > > > > >> > > > >>>> Sagar. >> > > > > >> > > > >>>> >> > > > > >> > > > >>>> >> > > > > >> > > > >>>> On Sat, May 28, 2022 at 8:19 PM Ismael Juma < >> > > > > ism...@juma.me.uk> >> > > > > >> > > > wrote: >> > > > > >> > > > >>>> >> > > > > >> > > > >>>>> Thanks for the KIP. The method makes sense, but the >> > name >> > > > is >> > > > > a >> > > > > >> bit >> > > > > >> > > > >> verbose. >> > > > > >> > > > >>>>> Have we considered a more concise name? >> > > > > >> > > > >>>>> >> > > > > >> > > > >>>>> Ismael >> > > > > >> > > > >>>>> >> > > > > >> > > > >>>>> On Tue, May 24, 2022, 4:49 AM Sagar < >> > > > > sagarmeansoc...@gmail.com >> > > > > >> > >> > > > > >> > > > >> wrote: >> > > > > >> > > > >>>>> >> > > > > >> > > > >>>>>> Hi All, >> > > > > >> > > > >>>>>> >> > > > > >> > > > >>>>>> I would like to open a voting thread for the >> > following >> > > > KIP: >> > > > > >> > > > >>>>>> >> > > > > >> > > > >>>>>> >> > > > > >> > > > >>>>>> >> > > > > >> > > > >>>>> >> > > > > >> > > > >> >> > > > > >> > > > >> > > > > >> > > >> > > > > >> > >> > > > > >> >> > > > > >> > > > >> > > >> > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-843%3A+Adding+metricOrElseCreate+method+to+Metrics >> > > > > >> > > > >>>>>> >> > > > > >> > > > >>>>>> Thanks! >> > > > > >> > > > >>>>>> Sagar. >> > > > > >> > > > >>>>>> >> > > > > >> > > > >>>>> >> > > > > >> > > > >>>> >> > > > > >> > > > >> >> > > > > >> > > > > >> > > > > >> > > > >> > > > > >> > > >> > > > > >> > >> > > > > >> >> > > > > >> >> > > > > >> -- >> > > > > >> -- Guozhang >> > > > > >> >> > > > > >> > > > >> > > >> > > >> > > -- >> > > -- Guozhang >> > > >> > >> >> >> -- >> -- Guozhang >> >
