Hi Artem, Thanks for the update to include motivation. Makes sense to me. Justine
On Wed, Jun 29, 2022 at 6:51 PM Luke Chen <show...@gmail.com> wrote: > Hi Artem, > > Thanks for the update. > LGTM. > > Luke > > On Thu, Jun 30, 2022 at 6:51 AM Artem Livshits > <alivsh...@confluent.io.invalid> wrote: > > > Thank you for your feedback. I've updated the KIP to elaborate on the > > motivation and provide some background on producer ids and how we measure > > them. > > > > Also, after some thinking and discussing it offline with some folks, I > > think that we don't really need partitioner level metrics. We can use > > existing tools to do granular debugging. I've moved partition level > > metrics to the rejected alternatives section. > > > > -Artem > > > > On Wed, Jun 29, 2022 at 1:57 AM Luke Chen <show...@gmail.com> wrote: > > > > > Hi Artem, > > > > > > Could you elaborate more in the motivation section? > > > I'm interested to know what kind of scenarios this metric can benefit > > for. > > > What could it bring to us when a topic partition has 100 > ProducerIdCount > > VS > > > another topic partition has 10 ProducerIdCount? > > > > > > Thank you. > > > Luke > > > > > > On Wed, Jun 29, 2022 at 6:30 AM Jun Rao <j...@confluent.io.invalid> > > wrote: > > > > > > > Hi, Artem, > > > > > > > > Thanks for the KIP. > > > > > > > > Could you explain the partition level ProducerIdCount a bit more? > Does > > > that > > > > reflect the number of PIDs ever produced to a partition since the > > broker > > > is > > > > started? Do we reduce the count after a PID expires? > > > > > > > > Thanks, > > > > > > > > Jun > > > > > > > > On Wed, Jun 22, 2022 at 1:08 AM David Jacot > > <dja...@confluent.io.invalid > > > > > > > > wrote: > > > > > > > > > Hi Artem, > > > > > > > > > > The KIP LGTM. > > > > > > > > > > Thanks, > > > > > David > > > > > > > > > > On Tue, Jun 21, 2022 at 9:32 PM Artem Livshits > > > > > <alivsh...@confluent.io.invalid> wrote: > > > > > > > > > > > > If there is no other feedback I'm going to start voting in a > couple > > > > days. > > > > > > > > > > > > -Artem > > > > > > > > > > > > On Fri, Jun 17, 2022 at 3:50 PM Artem Livshits < > > > alivsh...@confluent.io > > > > > > > > > > > wrote: > > > > > > > > > > > > > Thank you for your feedback. Updated the KIP and added the > > > Rejected > > > > > > > Alternatives section. > > > > > > > > > > > > > > -Artem > > > > > > > > > > > > > > On Fri, Jun 17, 2022 at 1:16 PM Ismael Juma <ism...@juma.me.uk > > > > > > wrote: > > > > > > > > > > > > > >> If we don't track them separately, then it makes sense to keep > > it > > > as > > > > > one > > > > > > >> metric. I'd probably name it ProducerIdCount in that case. > > > > > > >> > > > > > > >> Ismael > > > > > > >> > > > > > > >> On Fri, Jun 17, 2022 at 12:04 PM Artem Livshits > > > > > > >> <alivsh...@confluent.io.invalid> wrote: > > > > > > >> > > > > > > >> > Do you propose to have 2 metrics instead of one? Right now > we > > > > don't > > > > > > >> track > > > > > > >> > if the producer id was transactional or idempotent and for > > > metric > > > > > > >> > collection we'd either have to pay the cost of iterating > over > > > > > producer > > > > > > >> ids > > > > > > >> > (which could be a lot) or split the producer map into 2 or > > cache > > > > the > > > > > > >> > counts, which complicates the code. > > > > > > >> > > > > > > > >> > From the monitoring perspective, I think one metric should > be > > > > good, > > > > > but > > > > > > >> > maybe I'm missing some scenarios. > > > > > > >> > > > > > > > >> > -Artem > > > > > > >> > > > > > > > >> > On Fri, Jun 17, 2022 at 12:28 AM Ismael Juma < > > ism...@juma.me.uk > > > > > > > > > wrote: > > > > > > >> > > > > > > > >> > > I like the suggestion to have IdempotentProducerCount and > > > > > > >> > > TransactionalProducerCount metrics. > > > > > > >> > > > > > > > > >> > > Ismael > > > > > > >> > > > > > > > > >> > > On Thu, Jun 16, 2022 at 2:27 PM Artem Livshits > > > > > > >> > > <alivsh...@confluent.io.invalid> wrote: > > > > > > >> > > > > > > > > >> > > > Hi Ismael, > > > > > > >> > > > > > > > > > >> > > > Thank you for your feedback. Yes, this is counting the > > > number > > > > > of > > > > > > >> > > producer > > > > > > >> > > > ids tracked by the partition and broker. Another > options > > I > > > > was > > > > > > >> > thinking > > > > > > >> > > of > > > > > > >> > > > are the following: > > > > > > >> > > > > > > > > > >> > > > - IdempotentProducerCount > > > > > > >> > > > - TransactionalProducerCount > > > > > > >> > > > - ProducerIdCount > > > > > > >> > > > > > > > > > >> > > > Let me know if one of these seems better, or I'm open to > > > other > > > > > name > > > > > > >> > > > suggestions as well. > > > > > > >> > > > > > > > > > >> > > > -Artem > > > > > > >> > > > > > > > > > >> > > > On Wed, Jun 15, 2022 at 11:49 PM Ismael Juma < > > > > ism...@juma.me.uk > > > > > > > > > > > > >> > wrote: > > > > > > >> > > > > > > > > > >> > > > > Thanks for the KIP. > > > > > > >> > > > > > > > > > > >> > > > > ProducerCount seems like a misleading name since > > producers > > > > > > >> without a > > > > > > >> > > > > producer id are not counted. Is this meant to count > the > > > > > number of > > > > > > >> > > > producer > > > > > > >> > > > > IDs tracked by the broker? > > > > > > >> > > > > > > > > > > >> > > > > Ismael > > > > > > >> > > > > > > > > > > >> > > > > On Wed, Jun 15, 2022, 3:12 PM Artem Livshits < > > > > > > >> alivsh...@confluent.io > > > > > > >> > > > > .invalid> > > > > > > >> > > > > wrote: > > > > > > >> > > > > > > > > > > >> > > > > > Hello, > > > > > > >> > > > > > > > > > > > >> > > > > > I'd like to start a discussion on the KIP-847: > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-847%3A+Add+ProducerCount+metrics > > > > > > >> > > > > > . > > > > > > >> > > > > > > > > > > > >> > > > > > -Artem > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > >
