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 >
