Since the PR is reopened and we are going to re-merged the fixed PRs, what about just adding that as part of the KIP as the addendum?
On Fri, Feb 4, 2022 at 2:13 AM Sagar <sagarmeansoc...@gmail.com> wrote: > Thanks Sophie/Guozhang. > > Yeah I could have amended the KIP but it slipped my mind when Guozhang > proposed this in the PR. Later on, the PR was merged and KIP was marked as > adopted so I thought I will write a new one. I know the PR had been > reopened now :p . I dont have much preference on a new KIP v/s the original > one so anything is ok with me as well. > > I agree with the INFO part. I will make that change. > > Regarding task level, from my understanding, since every task's > buffer/cache size might be different so if a certain task might be > overshooting the limits then the task level metric might help people to > infer this. Also, thanks for the explanation Guozhang on why this should be > a task level metric. What are your thoughts on this @Sophie? > > Thanks! > Sagar. > > > On Fri, Feb 4, 2022 at 4:47 AM Guozhang Wang <wangg...@gmail.com> wrote: > > > Thanks Sagar for proposing the KIP, and Sophie for sharing your thoughts. > > Here're my 2c: > > > > I think I agree with Sophie for making the two metrics (both the added > and > > the newly proposed) on INFO level since we are always calculating them > > anyways. Regarding the level of the cache-size though, I'm thinking a bit > > different with you two: today we do not actually keep that caches on the > > per-store level, but rather on the per-thread level, i.e. when the cache > is > > full we would flush not only on the triggering state store but also > > potentially on other state stores as well of the task that thread owns. > > This mechanism, in hindsight, is a bit weird and we have some discussions > > about refactoring that in the future already. Personally I'd like to make > > this new metric to be aligned with whatever our future design will be. > > > > In the long run if we would not have a static assignment from tasks to > > threads, it may not make sense to keep a dedicated cache pool per thread. > > Instead all tasks will be dynamically sharing the globally configured max > > cache size (dynamically here means, we would not just divide the total > size > > by the num.tasks and then assign that to each task), and when a cache put > > triggers the flushing because the sum now exceeds the global configured > > value, we would potentially flush all the cached records for that task. > If > > this is the end stage, then I think keeping this metric at the task level > > is good. > > > > > > > > Guozhang > > > > > > > > > > On Thu, Feb 3, 2022 at 10:15 AM Sophie Blee-Goldman > > <sop...@confluent.io.invalid> wrote: > > > > > Hey Sagar, thanks for the KIP! > > > > > > And yes, all metrics are considered part of the public API and thus > > require > > > a KIP to add (or modify, etc...) Although in this particular case, you > > > could probably make a good case for just considering it as an update to > > the > > > original KIP which added the analogous metric > `input-buffer-bytes-total`. > > > For things like this that weren't considered during the KIP proposal > but > > > came up during the implementation or review, and are small changes that > > > would have made sense to include in that KIP had they been thought of, > > you > > > can just send an update to the existing KIP's discussion and.or voting > > > thread that explains what you want to add or modify and maybe a brief > > > description why. > > > > > > It's always ok to make a new KIP when in doubt, but there are some > cases > > > where an update email is sufficient. If there are any concerns or > > > suggestions that significantly expand the scope of the update, you can > > > always go create a new KIP and move the discussion there. > > > > > > I'd say you can feel free to proceed in whichever way you'd prefer for > > this > > > new proposal -- it just needs to appear in some KIP somewhere, and have > > > given the community thew opportunity to discuss and provide feedback > on. > > > > > > On that note, I do have two suggestions: > > > > > > 1) since we need to measure the size of the cache (and the input > > buffer(s) > > > anyways, we may as well make `cache-size-bytes-total` -- and also the > new > > > input-buffer-bytes-total -- an INFO level metric. In general the more > > > metrics the merrier, the only real reason for disabling some are if > they > > > have a performance impact or other cost that not everyone will want to > > pay. > > > In this case we're already computing the value of these metrics, so why > > not > > > expose it to the user as an INFO metric > > > 2) I think it would be both more natural and easier to implement if > this > > > was a store-level metric. A single task could in theory contain > multiple > > > physical state store caches and we would have to roll them up to report > > the > > > size for the task as a whole. It's additional work just to lose some > > > information that the user may want to have > > > > > > Let me know if anything here doesn't make sense or needs clarification. > > And > > > thanks for the quick followup to get this 2nd metric! > > > > > > -Sophie > > > > > > On Sat, Jan 29, 2022 at 4:27 AM Sagar <sagarmeansoc...@gmail.com> > wrote: > > > > > > > Hi All, > > > > > > > > I would like to open a discussion thread on the following KIP: > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=186878390 > > > > > > > > PS: This is about introducing a new metric and I am assuming that it > > > > requires a KIP. If that isn't the case, I can close it. > > > > > > > > Thanks! > > > > Sagar. > > > > > > > > > > > > > -- > > -- Guozhang > > > -- -- Guozhang
