+1 (non binding)
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 FetchMessageConversionsPerSecond 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 >> > > > > >> > > > > >> > > > >> > > >> > >>
