Hi Sophie, Sorry for the late comments. Thanks for the KIP.
One question from me: In test plan section, you mentioned we'll have benchmark for potential performance regression. And in the end, you said: See #2 under Rejected Alternatives for more on this. But I didn't find there's #2 in Rejected Alternatives section. I'm curious about it. Did you forget to put it? The KIP overall LGTM, as long as we won't create performance regression to it. Sorry for being late for the vote. Thank you. Luke On Thu, Jun 2, 2022 at 12:28 PM Sophie Blee-Goldman <sop...@confluent.io.invalid> wrote: > Thanks Guozhang -- I'll definitely make sure we have this benchmarked with > an eye for any regressions before it > makes it into a release. That said, if it's any comfort, we use the cached > system time and record only count and > sum type metrics, so unlike the *-rate *metrics for example we don't have > to make any calls to the system clock > which I know we have found to impact performance when investigating past > regressions. > > On Wed, Jun 1, 2022 at 11:07 AM Guozhang Wang <wangg...@gmail.com> wrote: > > > Thanks Sophie, that makes sense. Also I agree that since we are adding it > > at finest granularity for consumed metrics, it's better to have symmetry > > and add produced metrics at processor-node level as well. > > > > Regarding the benchmarks, I think that would be critical to have before > we > > add in the next release, since we propose to make this INFO level, > meaning > > it would be on by default, and collecting per-record is a critical path. > > > > > > Guozhang > > > > On Wed, Jun 1, 2022 at 8:04 AM Sophie Blee-Goldman > > <sop...@confluent.io.invalid> wrote: > > > > > I just want to send out a small update -- I decided to include the "- > > > *consumed*" metrics in the KIP alongside the > > > *"-produced"* metrics after all, for reasons I address in the > paragraph I > > > added at the end of the motivation section. > > > Please let me know if you have any questions or concerns > > > > > > Cheers, > > > Sophie > > > > > > On Wed, Jun 1, 2022 at 1:38 AM Sophie Blee-Goldman < > sop...@confluent.io> > > > wrote: > > > > > > > Just a quick question: for filling the gap of sub-topology > > visibilities, > > > >> would task-level produced metrics be sufficient? > > > > > > > > > > > > If I understand your question correctly, you're asking whether we > could > > > > just report at the task/subtopology > > > > level since we mainly want the bytes/throughput produced by the > > > > subtopology itself? > > > > > > > > Note that since we're only reporting this metric at the sink nodes, > > it's > > > > basically the same as a task-level metric > > > > for any subtopology that has only one sink and only scales with the > > > number > > > > of output topics. If you're concerned > > > > about a potential performance impact I would say that (a) making > these > > > > task-level doesn't buy us much, if anything, > > > > and (b) we can always revisit this if our benchmarks do indeed > reveal a > > > > regression but for now let's not over- > > > > optimize too much :) > > > > > > > > Also, the general philosophy behind the KAfka Streams metrics thus > far > > > > has been to report at the finest granularity > > > > and allow users to roll them up into whatever scope they want to > > > aggregate > > > > over. This KIP adopts the same approach. > > > > > > > > On Tue, May 31, 2022 at 12:02 PM Guozhang Wang <wangg...@gmail.com> > > > wrote: > > > > > > > >> Hi Sophie, > > > >> > > > >> Just a quick question: for filling the gap of sub-topology > > visibilities, > > > >> would task-level produced metrics be sufficient? > > > >> > > > >> On Mon, May 30, 2022 at 10:59 AM Bill Bejeck <bbej...@gmail.com> > > wrote: > > > >> > > > >> > Thanks for the KIP Sophie. > > > >> > > > > >> > I'm in favor of this change as well. I don't have any comments in > > > >> > addition to the ones already expressed. > > > >> > > > > >> > -Bill > > > >> > > > > >> > On Mon, May 30, 2022 at 4:55 AM Sagar <sagarmeansoc...@gmail.com> > > > >> wrote: > > > >> > > > > >> > > Hi Sophie, > > > >> > > > > > >> > > A very minor comment but you might want to remove this KIP > > template > > > >> > related > > > >> > > information from the top of the KIP: > > > >> > > > > > >> > > *This page is meant as a template for writing a KIP > > > >> > > < > > > >> > > > > > >> > > > > >> > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals > > > >> > > >. > > > >> > > To create a KIP choose Tools->Copy on this page and modify with > > your > > > >> > > content and replace the heading with the next KIP number and a > > > >> > description > > > >> > > of your issue. Replace anything in italics with your own > > > description.* > > > >> > > > > > >> > > > > > >> > > Thanks! > > > >> > > Sagar. > > > >> > > > > > >> > > On Mon, May 30, 2022 at 1:04 PM Sophie Blee-Goldman > > > >> > > <sop...@confluent.io.invalid> wrote: > > > >> > > > > > >> > > > > > > > >> > > > > Why does the title of the KIP talk about task-level metrics, > > but > > > >> the > > > >> > > > > specified metrics are on processor-level? > > > >> > > > > > > >> > > > > > > >> > > > Ah, my mistake -- it should indeed say "processor-level > > metrics". > > > >> > Thanks > > > >> > > > for the catch Bruno, the title has been fixed. > > > >> > > > > > > >> > > > Since there don't seem to be any concerns I'll proceed with > > > kicking > > > >> off > > > >> > > the > > > >> > > > vote. Thanks all! > > > >> > > > > > > >> > > > On Mon, May 30, 2022 at 12:01 AM Bruno Cadonna < > > > cado...@apache.org> > > > >> > > wrote: > > > >> > > > > > > >> > > > > Thanks for the KIP, Sophie! > > > >> > > > > > > > >> > > > > I am also in favor of this KIP! > > > >> > > > > > > > >> > > > > I have one minor question: > > > >> > > > > > > > >> > > > > Why does the title of the KIP talk about task-level metrics, > > but > > > >> the > > > >> > > > > specified metrics are on processor-level? > > > >> > > > > > > > >> > > > > For the rest, I am +1. > > > >> > > > > > > > >> > > > > Best, > > > >> > > > > Bruno > > > >> > > > > > > > >> > > > > On 29.05.22 00:20, John Roesler wrote: > > > >> > > > > > Thanks for the well motivated and documented KIP, Sophie! > > I’m > > > in > > > >> > > favor > > > >> > > > > of this change. > > > >> > > > > > > > > >> > > > > > -John > > > >> > > > > > > > > >> > > > > > On Sat, May 28, 2022, at 06:42, Sophie Blee-Goldman wrote: > > > >> > > > > >> Hey all, > > > >> > > > > >> > > > >> > > > > >> I'd like to propose a very small KIP to add two metrics > > that > > > >> will > > > >> > > help > > > >> > > > > fill > > > >> > > > > >> a gap in the derivable produced and consumed metrics. > > Please > > > >> take > > > >> > a > > > >> > > > look > > > >> > > > > >> and reply here with any questions or concerns. > > > >> > > > > >> > > > >> > > > > >> KIP-846: Task-level Streams metrics for bytes/records > > > Produced > > > >> > > > > >> < > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211886093 > > > >> > > > > > > > > >> > > > > >> > > > >> > > > > >> Given the small nature of this I'm going to call for a > vote > > > >> soon, > > > >> > > but > > > >> > > > > >> please don't hesitate to raise anything you feel should > be > > > >> > discussed > > > >> > > > in > > > >> > > > > >> more detail first. > > > >> > > > > >> > > > >> > > > > >> Thanks! > > > >> > > > > >> Sophie > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > >> -- > > > >> -- Guozhang > > > >> > > > > > > > > > > > > > -- > > -- Guozhang > > >
