Ok, thanks, leaving as is. On Wed, Sep 6, 2017 at 1:22 AM, Jason Gustafson <ja...@confluent.io> wrote:
> > > > I think I prefer the names with `Message` in them. For people less > familiar > > with Kafka, it makes it a bit clearer, I think. > > > Works for me. > > On Tue, Sep 5, 2017 at 5:19 PM, Ismael Juma <ism...@juma.me.uk> wrote: > > > I think I prefer the names with `Message` in them. For people less > familiar > > with Kafka, it makes it a bit clearer, I think. > > > > Ismael > > > > On Wed, Sep 6, 2017 at 12:39 AM, Rajini Sivaram <rajinisiva...@gmail.com > > > > wrote: > > > > > I am ok with dropping 'Message'. So the names would be > > > FetchConversionsPerSec, > > > ProduceConversionsPerSec and ConversionsTimeMs. The first two sound > fine. > > > Not so sure about ConversionsTimeMs, but since it appears with > > > Produce/Fetch as the request tag, it should be ok. I haven't updated > the > > > KIP yet. If there are no objections, I will update the KIP tomorrow. > > > > > > Regards, > > > > > > Rajini > > > > > > On Tue, Sep 5, 2017 at 7:23 PM, Jason Gustafson <ja...@confluent.io> > > > wrote: > > > > > > > > > > > > > I was wondering about the message versus record question. The fact > > that > > > > we > > > > > already have MessagesInPerSec seemed to favour the former. The > other > > > > aspect > > > > > is that for produce requests, we can up convert as well, so it > seemed > > > > > better to keep it generic. > > > > > > > > > > > > Yeah, so I thought maybe we could bypass the question and drop > > `Message` > > > > from the names if they were already clear enough. I'm fine with > either > > > way. > > > > > > > > On Tue, Sep 5, 2017 at 11:09 AM, Ismael Juma <ism...@juma.me.uk> > > wrote: > > > > > > > > > I was wondering about the message versus record question. The fact > > that > > > > we > > > > > already have MessagesInPerSec seemed to favour the former. The > other > > > > aspect > > > > > is that for produce requests, we can up convert as well, so it > seemed > > > > > better to keep it generic. > > > > > > > > > > Ismael > > > > > > > > > > On Tue, Sep 5, 2017 at 6:51 PM, Jason Gustafson < > ja...@confluent.io> > > > > > wrote: > > > > > > > > > > > +1 Lots of good stuff in here. > > > > > > > > > > > > One minor nit: in the name `FetchDownConversionsPerSec`, it's > > > implicit > > > > > that > > > > > > down-conversion is for messages. Could we do the same for > > > > > > `MessageConversionsTimeMs` and drop the `Message`? Then we don't > > have > > > > to > > > > > > decide if it should be 'Record' instead. > > > > > > > > > > > > On Tue, Sep 5, 2017 at 10:20 AM, Ismael Juma <ism...@juma.me.uk> > > > > wrote: > > > > > > > > > > > > > Thanks Rajini. > > > > > > > > > > > > > > 1. I meant a topic metric, but we could have one for fetch and > > one > > > > for > > > > > > > produce differentiated by the additional tag. The advantage is > > that > > > > the > > > > > > > name would be consistent with the request metric for message > > > > > conversions. > > > > > > > However, on closer inspection, this would make the name > > > inconsistent > > > > > with > > > > > > > the broker topic metrics: > > > > > > > > > > > > > > val totalProduceRequestRate = > > > > > > > newMeter(BrokerTopicStats.TotalProduceRequestsPerSec, > > "requests", > > > > > > > TimeUnit.SECONDS, tags) > > > > > > > val totalFetchRequestRate = > > > > > > > newMeter(BrokerTopicStats.TotalFetchRequestsPerSec, > "requests", > > > > > > > TimeUnit.SECONDS, tags) > > > > > > > > > > > > > > So, we maybe we can instead go for > FetchMessageConversionsPerSeco > > > nd > > > > > and > > > > > > > ProduceMessageConversionsPerSec. > > > > > > > > > > > > > > 2. Sounds good. > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > On Tue, Sep 5, 2017 at 5:46 PM, Rajini Sivaram < > > > > > rajinisiva...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Ismael, > > > > > > > > > > > > > > > > 1. At the moment FetchDownConversionsPerSec is a topic metric > > > while > > > > > > > > MessageConversionTimeMs is a request metric which indicates > > > > > > Produce/Fetch > > > > > > > > as a tag. Are you suggesting that we should convert > > > > > > > > FetchDownConversionsPerSec to a request metric called > > > > > > > > MessageConversionsPerSec > > > > > > > > for fetch requests? > > > > > > > > > > > > > > > > 2. TemporaryMessageSize for Produce/Fetch would indicate the > > > space > > > > > > > > allocated for conversions. For other requests, this metric > will > > > not > > > > > be > > > > > > > > created (unless we find a request where the size is large and > > > this > > > > > > > > information is useful). > > > > > > > > > > > > > > > > Thank you, > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Sep 5, 2017 at 4:55 PM, Ismael Juma < > ism...@juma.me.uk > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Thanks Rajini, +1 (binding) from me. Just a few minor > > comments: > > > > > > > > > > > > > > > > > > 1. FetchDownConversionsPerSec should probably be > > > > > > > MessageConversionsPerSec > > > > > > > > > with a request tag for consistency with > > > MessageConversionsTimeMs. > > > > > The > > > > > > > > text > > > > > > > > > in that paragraph should also be updated to talk about > > message > > > > > > > > conversions > > > > > > > > > instead of down conversions only. > > > > > > > > > > > > > > > > > > 2. What will TemporaryMemorySize represent for requests > other > > > > than > > > > > > > > > `ProduceRequest`? > > > > > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > > > > > On Mon, Sep 4, 2017 at 2:09 PM, Rajini Sivaram < > > > > > > > rajinisiva...@gmail.com> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > All the suggestions on the discuss thread have been > > > > incorporated > > > > > > into > > > > > > > > the > > > > > > > > > > KIP. Please let me know if you have any more concerns or > > else > > > > can > > > > > > we > > > > > > > > > > proceed with voting for this KIP? > > > > > > > > > > > > > > > > > > > > Thank you, > > > > > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > On Thu, Aug 24, 2017 at 6:50 PM, Rajini Sivaram < > > > > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > > > > > I would like to start the vote on KIP-188 that adds > > > > additional > > > > > > > > metrics > > > > > > > > > to > > > > > > > > > > > support health checks for Kafka Ops. Details are here: > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > > > > > > > 188+-+Add+new+metrics+to+support+health+checks > > > > > > > > > > > > > > > > > > > > > > Thank you, > > > > > > > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
