I agree with Allen. Go with the intuitive name, even if it means not deprecating. The impact of breakage here is small since it only breaks monitoring and the folks who watch their dashboards closely are the ones likely to read the release notes carefully and see this change.
On Wed, Mar 21, 2018, 3:24 PM Allen Wang <allenxw...@gmail.com> wrote: > I understand the impact to jmx based tools. But adding a new metric is > unnecessary for more advanced monitoring systems that can aggregate with or > without tags. Duplicating the metric (including the "request" tag) also > adds performance cost to the broker, especially for the metric that needs > to be updated for every request. > > For KIP-225, the choice makes sense because the deprecated metric's name is > undesirable anyway and the new metric name is much better than the prefixed > metric name. Not the case for RequestsPerSec. It is hard for me to come up > with another intuitive name. > > Thanks, > Allen > > > > On Wed, Mar 21, 2018 at 2:01 AM, Ted Yu <yuzhih...@gmail.com> wrote: > > > Creating new metric and deprecating existing one seems better from > > compatibility point of view. > > -------- Original message --------From: James Cheng < > wushuja...@gmail.com> > > Date: 3/21/18 1:39 AM (GMT-08:00) To: dev@kafka.apache.org Subject: > Re: > > [DISCUSS] KIP-272: Add API version tag to broker's RequestsPerSec metric > > Manikumar brings up a good point. This is a breaking change to the > > existing metric. Do we want to break compatibility, or do we want to add > a > > new metric and (optionally) deprecate the existing metric? > > > > For reference, in KIP-153 [1], we changed an existing metric without > doing > > proper deprecation. > > > > However, in KIP-225, we noticed that that we maybe shouldn't have done > > that. For KIP-225 [2], we instead decided to create a new metric, and > > deprecate (but not remove) the old one. > > > > -James > > > > [1] https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > 153%3A+Include+only+client+traffic+in+BytesOutPerSec+metric > > > > [2] https://cwiki.apache.org/confluence/pages/viewpage. > > action?pageId=74686649 > > > > > > > On Mar 21, 2018, at 12:14 AM, Manikumar <manikumar.re...@gmail.com> > > wrote: > > > > > > Can we retain total RequestsPerSec metric and add new version tag > metric? > > > When monitoring with simple jconsole/jmx based tools, It is useful to > > have > > > total metric > > > to monitor request rate. > > > > > > > > > Thanks, > > > > > > On Wed, Mar 21, 2018 at 11:01 AM, Gwen Shapira <g...@confluent.io> > > wrote: > > > > > >> I love this. Not much to add - it is an elegant solution, clean > > >> implementation and it addresses a real need, especially during > upgrades. > > >> > > >> On Tue, Mar 20, 2018 at 2:49 PM, Ted Yu <yuzhih...@gmail.com> wrote: > > >> > > >>> Thanks for the response. > > >>> > > >>> Assuming number of client versions is limited in a cluster, memory > > >>> consumption is not a concern. > > >>> > > >>> Cheers > > >>> > > >>> On Tue, Mar 20, 2018 at 10:47 AM, Allen Wang <allenxw...@gmail.com> > > >> wrote: > > >>> > > >>>> Hi Ted, > > >>>> > > >>>> The additional hash map is very small, possibly a few KB. Each > request > > >>> type > > >>>> ("produce", "fetch", etc.) will have such a map which have a few > > >> entries > > >>>> depending on the client API versions the broker will encounter. So > if > > >>>> broker encounters two client versions for "produce", there will be > two > > >>>> entries in the map for "produce" requests mapping from version to > > >> meter. > > >>> Of > > >>>> course, hash map always have additional memory overhead. > > >>>> > > >>>> Thanks, > > >>>> Allen > > >>>> > > >>>> > > >>>> On Mon, Mar 19, 2018 at 3:49 PM, Ted Yu <yuzhih...@gmail.com> > wrote: > > >>>> > > >>>>> bq. *additional hash lookup is needed when updating the metric to > > >>> locate > > >>>>> the metric * > > >>>>> > > >>>>> *Do you have estimate how much memory is needed for maintaining the > > >>> hash > > >>>>> map ?* > > >>>>> > > >>>>> *Thanks* > > >>>>> > > >>>>> On Mon, Mar 19, 2018 at 3:19 PM, Allen Wang <allenxw...@gmail.com> > > >>>> wrote: > > >>>>> > > >>>>>> Hi all, > > >>>>>> > > >>>>>> I have created KIP-272: Add API version tag to broker's > > >>> RequestsPerSec > > >>>>>> metric. > > >>>>>> > > >>>>>> Here is the link to the KIP: > > >>>>>> > > >>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > >>>>>> 272%3A+Add+API+version+tag+to+broker%27s+RequestsPerSec+metric > > >>>>>> > > >>>>>> Looking forward to the discussion. > > >>>>>> > > >>>>>> Thanks, > > >>>>>> Allen > > >>>>>> > > >>>>> > > >>>> > > >>> > > >> > > >> > > >> > > >> -- > > >> *Gwen Shapira* > > >> Product Manager | Confluent > > >> 650.450.2760 | @gwenshap > > >> Follow us: Twitter <https://twitter.com/ConfluentInc> | blog > > >> <http://www.confluent.io/blog> > > >> > > > > >
