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