Actually, one other point: our existing state store operation metrics are measured in nanoseconds[1].
Should iterator-duration-(avg|max) also be measured in nanoseconds, for consistency, or should we keep them milliseconds, as the KIP currently states? 1: https://docs.confluent.io/platform/current/streams/monitoring.html#state-store-metrics On Thu, 16 May 2024 at 12:15, Nick Telford <nick.telf...@gmail.com> wrote: > Good point! I've updated it to "Improved StateStore Iterator metrics for > detecting leaks" - let me know if you have a better suggestion. > > This should affect the voting imo, as nothing of substance has changed. > > Regards, > Nick > > On Thu, 16 May 2024 at 01:39, Sophie Blee-Goldman <sop...@responsive.dev> > wrote: > >> One quick thing -- can you update the title of this KIP to reflect the >> decision to implement these metrics for all state stores implementations >> rather than just RocksDB? >> >> >> On Tue, May 14, 2024 at 1:36 PM Nick Telford <nick.telf...@gmail.com> >> wrote: >> >> > Woops! Thanks for the catch Lucas. Given this was just a typo, I don't >> > think this affects the voting. >> > >> > Cheers, >> > Nick >> > >> > On Tue, 14 May 2024 at 18:06, Lucas Brutschy <lbruts...@confluent.io >> > .invalid> >> > wrote: >> > >> > > Hi Nick, >> > > >> > > you are still referring to oldest-open-iterator-age-ms in the >> > > `Proposed Changes` section. >> > > >> > > Cheers, >> > > Lucas >> > > >> > > On Thu, May 2, 2024 at 4:00 PM Lucas Brutschy <lbruts...@confluent.io >> > >> > > wrote: >> > > > >> > > > Hi Nick! >> > > > >> > > > I agree, the age variant is a bit nicer since the semantics are very >> > > > clear from the name. If you'd rather go for the simple >> implementation, >> > > > how about calling it `oldest-iterator-open-since-ms`? I believe this >> > > > could be understood without docs. Either way, I think we should be >> > > > able to open the vote for this KIP because nobody raised any major / >> > > > blocking concerns. >> > > > >> > > > Looking forward to getting this voted on soon! >> > > > >> > > > Cheers >> > > > Lucas >> > > > >> > > > On Sun, Mar 31, 2024 at 5:23 PM Nick Telford < >> nick.telf...@gmail.com> >> > > wrote: >> > > > > >> > > > > Hi Matthias, >> > > > > >> > > > > > For the oldest iterator metric, I would propose something simple >> > like >> > > > > > `iterator-opened-ms` and it would just be the actual timestamp >> when >> > > the >> > > > > > iterator was opened. I don't think we need to compute the actual >> > age, >> > > > > > but user can to this computation themselves? >> > > > > >> > > > > That works for me; it's easier to implement like that :-D I'm a >> > little >> > > > > concerned that the name "iterator-opened-ms" may not be obvious >> > enough >> > > > > without reading the docs. >> > > > > >> > > > > > If we think reporting the age instead of just the timestamp is >> > > better, I >> > > > > > would propose `iterator-max-age-ms`. I should be sufficient to >> call >> > > out >> > > > > > (as it's kinda "obvious" anyway) that the metric applies to open >> > > > > > iterator only. >> > > > > >> > > > > While I think it's preferable to record the timestamp, rather than >> > the >> > > age, >> > > > > this does have the benefit of a more obvious metric name. >> > > > > >> > > > > > Nit: the KIP says it's a store-level metric, but I think it >> would >> > be >> > > > > > good to say explicitly that it's recorded with DEBUG level only? >> > > > > >> > > > > Yes, I've already updated the KIP with this information in the >> table. >> > > > > >> > > > > Regards, >> > > > > >> > > > > Nick >> > > > > >> > > > > On Sun, 31 Mar 2024 at 10:53, Matthias J. Sax <mj...@apache.org> >> > > wrote: >> > > > > >> > > > > > The time window thing was just an idea. Happy to drop it. >> > > > > > >> > > > > > For the oldest iterator metric, I would propose something simple >> > like >> > > > > > `iterator-opened-ms` and it would just be the actual timestamp >> when >> > > the >> > > > > > iterator was opened. I don't think we need to compute the actual >> > age, >> > > > > > but user can to this computation themselves? >> > > > > > >> > > > > > If we think reporting the age instead of just the timestamp is >> > > better, I >> > > > > > would propose `iterator-max-age-ms`. I should be sufficient to >> call >> > > out >> > > > > > (as it's kinda "obvious" anyway) that the metric applies to open >> > > > > > iterator only. >> > > > > > >> > > > > > And yes, I was hoping that the code inside MetereXxxStore might >> > > already >> > > > > > be setup in a way that custom stores would inherit the iterator >> > > metrics >> > > > > > automatically -- I am just not sure, and left it as an exercise >> for >> > > > > > somebody to confirm :) >> > > > > > >> > > > > > >> > > > > > Nit: the KIP says it's a store-level metric, but I think it >> would >> > be >> > > > > > good to say explicitly that it's recorded with DEBUG level only? >> > > > > > >> > > > > > >> > > > > > >> > > > > > -Matthias >> > > > > > >> > > > > > >> > > > > > On 3/28/24 2:52 PM, Nick Telford wrote: >> > > > > > > Quick addendum: >> > > > > > > >> > > > > > > My suggested metric "oldest-open-iterator-age-seconds" should >> be >> > > > > > > "oldest-open-iterator-age-ms". Milliseconds is obviously a >> better >> > > > > > > granularity for such a metric. >> > > > > > > >> > > > > > > Still accepting suggestions for a better name. >> > > > > > > >> > > > > > > On Thu, 28 Mar 2024 at 13:41, Nick Telford < >> > nick.telf...@gmail.com >> > > > >> > > > > > wrote: >> > > > > > > >> > > > > > >> Hi everyone, >> > > > > > >> >> > > > > > >> Sorry for leaving this for so long. So much for "3 weeks >> until >> > KIP >> > > > > > freeze"! >> > > > > > >> >> > > > > > >> On Sophie's comments: >> > > > > > >> 1. Would Matthias's suggestion of a separate metric tracking >> the >> > > age of >> > > > > > >> the oldest open iterator (within the tag set) satisfy this? >> That >> > > way we >> > > > > > can >> > > > > > >> keep iterator-duration-(avg|max) for closed iterators, which >> can >> > > be >> > > > > > useful >> > > > > > >> for performance debugging for iterators that don't leak. I'm >> not >> > > sure >> > > > > > what >> > > > > > >> we'd call this metric, maybe: >> > "oldest-open-iterator-age-seconds"? >> > > Seems >> > > > > > >> like a mouthful. >> > > > > > >> >> > > > > > >> 2. You're right, it makes more sense to provide >> > > > > > >> iterator-duration-(avg|max). Honestly, I can't remember why I >> > had >> > > > > > "total" >> > > > > > >> before, or why I was computing a rate-of-change over it. >> > > > > > >> >> > > > > > >> 3, 4, 5, 6. Agreed, I'll make all those changes as suggested. >> > > > > > >> >> > > > > > >> 7. Combined with Matthias's point about RocksDB, I'm >> convinced >> > > that this >> > > > > > >> is the wrong KIP for these. I'll introduce the additional >> Rocks >> > > metrics >> > > > > > in >> > > > > > >> another KIP. >> > > > > > >> >> > > > > > >> On Matthias's comments: >> > > > > > >> A. Not sure about the time window. I'm pretty sure all >> existing >> > > avg/max >> > > > > > >> metrics are since the application was started? Any other >> > > suggestions >> > > > > > here >> > > > > > >> would be appreciated. >> > > > > > >> >> > > > > > >> B. Agreed. See point 1 above. >> > > > > > >> >> > > > > > >> C. Good point. My focus was very much on Rocks memory leaks >> when >> > > I wrote >> > > > > > >> the first draft. I can generalise it. My only concern is >> that it >> > > might >> > > > > > make >> > > > > > >> it more difficult to detect Rocks iterator leaks caused >> *within* >> > > our >> > > > > > >> high-level iterator, e.g. RocksJNI, RocksDB, RocksDBStore, >> etc. >> > > But we >> > > > > > >> could always provide a RocksDB-specific metric for this, as >> you >> > > > > > suggested. >> > > > > > >> >> > > > > > >> D. Hmm, we do already have MeteredKeyValueIterator, which >> > > automatically >> > > > > > >> wraps the iterator from inner-stores of >> MeteredKeyValueStore. If >> > > we >> > > > > > >> implemented these metrics there, then custom stores would >> > > automatically >> > > > > > >> gain the functionality, right? This seems like a pretty >> logical >> > > place to >> > > > > > >> implement these metrics, since MeteredKeyValueStore is all >> about >> > > adding >> > > > > > >> metrics to state stores. >> > > > > > >> >> > > > > > >>> I imagine the best way to implement this would be to do so >> at >> > the >> > > > > > >>> high-level iterator rather than implementing it separately >> for >> > > each >> > > > > > >>> specific iterator implementation for every store type. >> > > > > > >> >> > > > > > >> Sophie, does MeteredKeyValueIterator fit with your >> > recommendation? >> > > > > > >> >> > > > > > >> Thanks for your thoughts everyone, I'll update the KIP now. >> > > > > > >> >> > > > > > >> Nick >> > > > > > >> >> > > > > > >> On Thu, 14 Mar 2024 at 03:37, Sophie Blee-Goldman < >> > > > > > sop...@responsive.dev> >> > > > > > >> wrote: >> > > > > > >> >> > > > > > >>> About your last two points: I completely agree that we >> should >> > > try to >> > > > > > >>> make this independent of RocksDB, and should probably adopt >> a >> > > > > > >>> general philosophy of being store-implementation agnostic >> > unless >> > > > > > >>> there is good reason to focus on a particular store type: >> eg if >> > > it was >> > > > > > >>> only possible to implement for certain stores, or only made >> > > sense in >> > > > > > >>> the context of a certain store type but not necessarily >> stores >> > in >> > > > > > general. >> > > > > > >>> >> > > > > > >>> While leaking memory due to unclosed iterators on RocksDB >> > stores >> > > is >> > > > > > >>> certainly the most common issue, I think Matthias >> sufficiently >> > > > > > >>> demonstrated that the problem of leaking iterators is not >> > > actually >> > > > > > >>> unique to RocksDB, and we should consider including >> in-memory >> > > > > > >>> stores at the very least. I also think that at this point, >> we >> > > may as >> > > > > > well >> > > > > > >>> just implement the metrics for *all* store types, whether >> > > rocksdb or >> > > > > > >>> in-memory or custom. Not just because it probably applies to >> > all >> > > > > > >>> store types (leaking iterators are rarely a good thing!) but >> > > because >> > > > > > >>> I imagine the best way to implement this would be to do so >> at >> > the >> > > > > > >>> high-level iterator rather than implementing it separately >> for >> > > each >> > > > > > >>> specific iterator implementation for every store type. >> > > > > > >>> >> > > > > > >>> That said, I haven't thought all that carefully about the >> > > > > > implementation >> > > > > > >>> yet -- it just strikes me as easiest to do at the top level >> of >> > > the >> > > > > > store >> > > > > > >>> hierarchy rather than at the bottom. My gut instinct may >> very >> > > well be >> > > > > > >>> wrong, but that's what it's saying >> > > > > > >>> >> > > > > > >>> On Thu, Mar 7, 2024 at 10:43 AM Matthias J. Sax < >> > > mj...@apache.org> >> > > > > > wrote: >> > > > > > >>> >> > > > > > >>>> Seems I am late to this party. Can we pick this up again >> > aiming >> > > for >> > > > > > 3.8 >> > > > > > >>>> release? I think it would be a great addition. Few >> comments: >> > > > > > >>>> >> > > > > > >>>> >> > > > > > >>>> - I think it does make sense to report >> `iterator-duration-avg` >> > > and >> > > > > > >>>> `iterator-duration-max` for all *closed* iterators -- it >> just >> > > seems to >> > > > > > >>>> be a useful metric (wondering if this would be _overall_ or >> > > bounded to >> > > > > > >>>> some time window?) >> > > > > > >>>> >> > > > > > >>>> - About the duration iterators are currently open, I >> believe >> > > the only >> > > > > > >>>> useful way is to report the "oldest iterator", ie, the >> > smallest >> > > > > > iterator >> > > > > > >>>> open-time, of all currently open-iterator? We all agree >> that >> > in >> > > > > > general, >> > > > > > >>>> leaking iterator would bump the count metric, and if there >> is >> > a >> > > few >> > > > > > ones >> > > > > > >>>> which are not closed and open for a long time, it seem >> > > sufficient to >> > > > > > >>>> detect the single oldest one for alerting purpose? >> > > > > > >>>> >> > > > > > >>>> - What I don't like about the KIP is it focus on RocksDB. I >> > > don't >> > > > > > think >> > > > > > >>>> we should build on the internal RocksDB counters as >> proposed >> > (I >> > > guess, >> > > > > > >>>> we could still expose them, similar to other RocksDB >> metrics >> > > which we >> > > > > > >>>> expose already). However, for this new metric, we should >> track >> > > it >> > > > > > >>>> ourselves and thus make it independent of RocksDB -- in the >> > > end, an >> > > > > > >>>> in-memory store could also leak memory (and kill a JVM >> with an >> > > > > > >>>> out-of-memory error) and we should be able to track it. >> > > > > > >>>> >> > > > > > >>>> - Not sure if we would like to add support for custom >> stores, >> > > to allow >> > > > > > >>>> them to register their iterators with this metric? Or would >> > > this not >> > > > > > be >> > > > > > >>>> necessary, because custom stores could just register a >> custom >> > > metric >> > > > > > >>>> about it to begin with? >> > > > > > >>>> >> > > > > > >>>> >> > > > > > >>>> >> > > > > > >>>> -Matthias >> > > > > > >>>> >> > > > > > >>>> On 10/25/23 4:41 PM, Sophie Blee-Goldman wrote: >> > > > > > >>>>>> >> > > > > > >>>>>> If we used "iterator-duration-max", for >> > > > > > >>>>>> example, would it not be confusing that it includes >> > Iterators >> > > that >> > > > > > >>> are >> > > > > > >>>>>> still open, and therefore the duration is not yet known? >> > > > > > >>>>> >> > > > > > >>>>> >> > > > > > >>>>> 1. Ah, I think I understand your concern better now -- I >> > > totally >> > > > > > agree >> > > > > > >>>> that >> > > > > > >>>>> a >> > > > > > >>>>> "iterator-duration-max" metric would be >> > > confusing/misleading. I >> > > > > > was >> > > > > > >>>>> thinking about it a bit differently, something more akin >> to >> > the >> > > > > > >>>>> "last-rebalance-seconds-ago" consumer metric. As the name >> > > suggests, >> > > > > > >>>>> that basically just tracks how long the consumer has gone >> > > without >> > > > > > >>>>> rebalancing -- it doesn't purport to represent the actual >> > > duration >> > > > > > >>>> between >> > > > > > >>>>> rebalances, just the current time since the last one. The >> > > hard part >> > > > > > >>> is >> > > > > > >>>>> really >> > > > > > >>>>> in choosing a name that reflects this -- maybe you have >> some >> > > better >> > > > > > >>> ideas >> > > > > > >>>>> but off the top of my head, perhaps something like >> > > > > > >>>> "iterator-lifetime-max"? >> > > > > > >>>>> >> > > > > > >>>>> 2. I'm not quite sure how to interpret the >> > > "iterator-duration-total" >> > > > > > >>>> metric >> > > > > > >>>>> -- what exactly does it mean to add up all the iterator >> > > durations? >> > > > > > For >> > > > > > >>>>> some context, while this is not a hard-and-fast rule, in >> > > general >> > > > > > >>> you'll >> > > > > > >>>>> find that Kafka/Streams metrics tend to come in pairs of >> > > avg/max or >> > > > > > >>>>> rate/total. Something that you might measure the avg for >> > > usually is >> > > > > > >>>>> also useful to measure the max, whereas a total metric is >> > > probably >> > > > > > >>>>> also useful as a rate but not so much as an avg. I >> actually >> > > think >> > > > > > this >> > > > > > >>>>> is part of why it feels like it makes so much sense to >> > include >> > > a >> > > > > > "max" >> > > > > > >>>>> version of this metric, as Lucas suggested, even if the >> name >> > of >> > > > > > >>>>> "iterator-duration-max" feels misleading. Ultimately the >> > > metric names >> > > > > > >>>>> are up to you, but for this reason, I would personally >> > > advocate for >> > > > > > >>>>> just going with an "iterator-duration-avg" and >> > > > > > "iterator-duration-max" >> > > > > > >>>>> >> > > > > > >>>>> I did see your example in which you mention one could >> monitor >> > > the >> > > > > > >>>>> rate of change of the "-total" metric. While this does >> make >> > > sense to >> > > > > > >>>>> me, if the only way to interpret a metric is by computing >> > > another >> > > > > > >>>>> function over it, then why not just make that computation >> the >> > > metric >> > > > > > >>>>> and cut out the middle man? And in this case, to me at >> least, >> > > it >> > > > > > feels >> > > > > > >>>>> much easier to understand a metric like >> > > "iterator-duration-max" vs >> > > > > > >>>>> something like "iterator-duration-total-rate" >> > > > > > >>>>> >> > > > > > >>>>> 3. By the way, can you add another column to the table >> with >> > > the new >> > > > > > >>>> metrics >> > > > > > >>>>> that lists the recording level? My suggestion would be to >> put >> > > the >> > > > > > >>>>> "number-open-iterators" at INFO and the other two at >> DEBUG. >> > See >> > > > > > >>>>> the following for my reasoning behind this recommendation >> > > > > > >>>>> >> > > > > > >>>>> 4. I would change the "Type" entry for the >> > > "number-open-iterators" >> > > > > > >>> from >> > > > > > >>>>> "Value" to "Gauge". This helps justify the "INFO" level >> for >> > > this >> > > > > > >>> metric, >> > > > > > >>>>> since unlike the other metrics which are "Measurables", >> the >> > > current >> > > > > > >>>>> timestamp won't need to be retrieved on each recording >> > > > > > >>>>> >> > > > > > >>>>> 5. Can you list the tags that would be associated with >> each >> > of >> > > these >> > > > > > >>>>> metrics (either in the table, or separately above/below if >> > > they will >> > > > > > >>> be >> > > > > > >>>>> the same for all) >> > > > > > >>>>> >> > > > > > >>>>> 6. Do you have a strong preference for the name >> > > > > > >>> "number-open-iterators" >> > > > > > >>>>> or would you be alright in shortening this to >> > > "num-open-iterators"? >> > > > > > >>> The >> > > > > > >>>>> latter is more in line with the naming scheme used >> elsewhere >> > > in Kafka >> > > > > > >>>>> for similar kinds of metrics, and a shorter name is always >> > > nice. >> > > > > > >>>>> >> > > > > > >>>>> 7. With respect to the rocksdb cache metrics, those sound >> > > useful but >> > > > > > >>>>> if it was me, I would probably save them for a separate >> KIP >> > > mainly >> > > > > > >>> just >> > > > > > >>>>> because the KIP freeze deadline is in a few weeks, and I >> > > wouldn't >> > > > > > want >> > > > > > >>>>> to end up blocking all the new metrics just because there >> was >> > > ongoing >> > > > > > >>>>> debate about a subset of them. That said, you do have 3 >> full >> > > weeks, >> > > > > > so >> > > > > > >>>>> I would hope that you could get both sets of metrics >> agreed >> > > upon in >> > > > > > >>>>> that timeframe! >> > > > > > >>>>> >> > > > > > >>>>> >> > > > > > >>>>> On Tue, Oct 24, 2023 at 6:35 AM Nick Telford < >> > > nick.telf...@gmail.com >> > > > > > > >> > > > > > >>>> wrote: >> > > > > > >>>>> >> > > > > > >>>>>> I don't really have a problem with adding such a metric, >> I'm >> > > just >> > > > > > not >> > > > > > >>>>>> entirely sure how it would work. If we used >> > > "iterator-duration-max", >> > > > > > >>> for >> > > > > > >>>>>> example, would it not be confusing that it includes >> > Iterators >> > > that >> > > > > > >>> are >> > > > > > >>>>>> still open, and therefore the duration is not yet known? >> > When >> > > > > > >>> graphing >> > > > > > >>>> that >> > > > > > >>>>>> over time, I suspect it would be difficult to understand. >> > > > > > >>>>>> >> > > > > > >>>>>> 3. >> > > > > > >>>>>> FWIW, this would still be picked up by "open-iterators", >> > > since that >> > > > > > >>>> metric >> > > > > > >>>>>> is only decremented when Iterator#close is called (via >> the >> > > > > > >>>>>> ManagedKeyValueIterator#onClose hook). >> > > > > > >>>>>> >> > > > > > >>>>>> I'm actually considering expanding the scope of this KIP >> > > slightly to >> > > > > > >>>>>> include improved Block Cache metrics, as my own memory >> leak >> > > > > > >>>> investigations >> > > > > > >>>>>> have trended in that direction. Do you think the >> following >> > > metrics >> > > > > > >>>> should >> > > > > > >>>>>> be included in this KIP, or should I create a new KIP? >> > > > > > >>>>>> >> > > > > > >>>>>> - block-cache-index-usage (number of bytes occupied >> by >> > > index >> > > > > > >>> blocks) >> > > > > > >>>>>> - block-cache-filter-usage (number of bytes >> occupied by >> > > filter >> > > > > > >>>> blocks) >> > > > > > >>>>>> >> > > > > > >>>>>> Regards, >> > > > > > >>>>>> Nick >> > > > > > >>>>>> >> > > > > > >>>>>> On Tue, 24 Oct 2023 at 07:09, Sophie Blee-Goldman < >> > > > > > >>>> sop...@responsive.dev> >> > > > > > >>>>>> wrote: >> > > > > > >>>>>> >> > > > > > >>>>>>> I actually think we could implement Lucas' suggestion >> > pretty >> > > easily >> > > > > > >>> and >> > > > > > >>>>>>> without too much additional effort. We have full control >> > > over the >> > > > > > >>>>>> iterator >> > > > > > >>>>>>> that is returned by the various range queries, so it >> would >> > > be easy >> > > > > > >>> to >> > > > > > >>>>>>> register a gauge metric for how long it has been since >> the >> > > iterator >> > > > > > >>> was >> > > > > > >>>>>>> created. Then we just deregister the metric when the >> > > iterator is >> > > > > > >>>> closed. >> > > > > > >>>>>>> >> > > > > > >>>>>>> With respect to how useful this metric would be, both >> Nick >> > > and >> > > > > > Lucas >> > > > > > >>>> have >> > > > > > >>>>>>> made good points: I would agree that in general, leaking >> > > iterators >> > > > > > >>>> would >> > > > > > >>>>>>> mean an ever-increasing iterator count that should be >> > > possible to >> > > > > > >>> spot >> > > > > > >>>>>>> without this. However, a few things to consider: >> > > > > > >>>>>>> >> > > > > > >>>>>>> 1. it's really easy to set up an alert based on some >> > maximum >> > > > > > >>> threshold >> > > > > > >>>> of >> > > > > > >>>>>>> how long an iterator should remain open for. It's >> > relatively >> > > more >> > > > > > >>>> tricky >> > > > > > >>>>>> to >> > > > > > >>>>>>> set up alerts based on the current count of open >> iterators >> > > and how >> > > > > > >>> it >> > > > > > >>>>>>> changes over time. >> > > > > > >>>>>>> 2. As Lucas mentioned, it only takes a few iterators to >> > > wreak havoc >> > > > > > >>> in >> > > > > > >>>>>>> extreme cases. Sometimes more advanced applications end >> up >> > > with >> > > > > > >>> just a >> > > > > > >>>>>> few >> > > > > > >>>>>>> leaking iterators despite closing the majority of them. >> > I've >> > > seen >> > > > > > >>> this >> > > > > > >>>>>>> happen just once personally, but it was driving everyone >> > > crazy >> > > > > > >>> until we >> > > > > > >>>>>>> figured it out. >> > > > > > >>>>>>> 3. A metric for how long the iterator has been open >> would >> > > help to >> > > > > > >>>>>> identify >> > > > > > >>>>>>> hanging iterators due to some logic where the iterator >> is >> > > properly >> > > > > > >>>> closed >> > > > > > >>>>>>> but for whatever reason just isn't being advanced to the >> > > end, and >> > > > > > >>> thus >> > > > > > >>>>>> not >> > > > > > >>>>>>> reached the iterator#close line of the user code. This >> case >> > > seems >> > > > > > >>>>>> difficult >> > > > > > >>>>>>> to spot without the specific metric for iterator >> lifetime >> > > > > > >>>>>>> 4. Lastly, I think you could argue that all of the above >> > are >> > > fairly >> > > > > > >>>>>>> advanced use cases, but this seems like a fairly >> advanced >> > > feature >> > > > > > >>>>>> already, >> > > > > > >>>>>>> so it doesn't seem unreasonable to try and cover all the >> > > bases. >> > > > > > >>>>>>> >> > > > > > >>>>>>> All that said, my philosophy is that the KIP author gets >> > the >> > > final >> > > > > > >>> word >> > > > > > >>>>>> on >> > > > > > >>>>>>> what to pull into scope as long as the proposal isn't >> > harming >> > > > > > anyone >> > > > > > >>>>>>> without the extra feature/changes. So it's up to you >> Nick >> > > -- just >> > > > > > >>>> wanted >> > > > > > >>>>>>> to add some context on how it could work, and why it >> would >> > be >> > > > > > >>> helpful >> > > > > > >>>>>>> >> > > > > > >>>>>>> Thanks for the KIP! >> > > > > > >>>>>>> >> > > > > > >>>>>>> On Wed, Oct 18, 2023 at 7:04 AM Lucas Brutschy >> > > > > > >>>>>>> <lbruts...@confluent.io.invalid> wrote: >> > > > > > >>>>>>> >> > > > > > >>>>>>>> Hi Nick, >> > > > > > >>>>>>>> >> > > > > > >>>>>>>> I did not think in detail about how to implement it, >> just >> > > about >> > > > > > >>> what >> > > > > > >>>>>>>> metrics would be nice to have. You are right, we'd >> have to >> > > > > > >>>>>>>> register/deregister the iterators during open/close. >> This >> > > would be >> > > > > > >>>>>>>> more complicated to implement than the other metrics, >> but >> > I >> > > do not >> > > > > > >>> see >> > > > > > >>>>>>>> a fundamental problem with it. >> > > > > > >>>>>>>> >> > > > > > >>>>>>>> As far as I understand, even a low number of leaked >> > > iterators can >> > > > > > >>> hurt >> > > > > > >>>>>>>> RocksDB compaction significantly. So we may even want >> to >> > > detect if >> > > > > > >>> the >> > > > > > >>>>>>>> iterators are opened by one-time / rare queries against >> > the >> > > state >> > > > > > >>>>>>>> store. >> > > > > > >>>>>>>> >> > > > > > >>>>>>>> But, as I said, that would be an addition and not a >> change >> > > of the >> > > > > > >>>>>>>> current contents of the KIP, so I'd support the KIP >> moving >> > > forward >> > > > > > >>>>>>>> even without this extension. >> > > > > > >>>>>>>> >> > > > > > >>>>>>>> Cheers, Lucas >> > > > > > >>>>>>>> >> > > > > > >>>>>>>> >> > > > > > >>>>>>>> >> > > > > > >>>>>>>> On Tue, Oct 17, 2023 at 3:45 PM Nick Telford < >> > > > > > >>> nick.telf...@gmail.com> >> > > > > > >>>>>>>> wrote: >> > > > > > >>>>>>>>> >> > > > > > >>>>>>>>> Hi Lucas, >> > > > > > >>>>>>>>> >> > > > > > >>>>>>>>> Hmm, I'm not sure how we could reliably identify such >> > > leaked >> > > > > > >>>>>> Iterators. >> > > > > > >>>>>>>> If >> > > > > > >>>>>>>>> we tried to include open iterators when calculating >> > > > > > >>>>>> iterator-duration, >> > > > > > >>>>>>>> we'd >> > > > > > >>>>>>>>> need some kind of registry of all the open iterator >> > > creation >> > > > > > >>>>>>> timestamps, >> > > > > > >>>>>>>>> wouldn't we? >> > > > > > >>>>>>>>> >> > > > > > >>>>>>>>> In general, if you have a leaky Iterator, it should >> > > manifest as a >> > > > > > >>>>>>>>> persistently climbing "open-iterators" metric, even >> on a >> > > busy >> > > > > > >>> node, >> > > > > > >>>>>>>> because >> > > > > > >>>>>>>>> each time that Iterator is used, it will leak another >> > one. >> > > So >> > > > > > >>> even in >> > > > > > >>>>>>> the >> > > > > > >>>>>>>>> presence of many non-leaky Iterators on a busy >> instance, >> > > the >> > > > > > >>> metric >> > > > > > >>>>>>>> should >> > > > > > >>>>>>>>> still consistently climb. >> > > > > > >>>>>>>>> >> > > > > > >>>>>>>>> Regards, >> > > > > > >>>>>>>>> >> > > > > > >>>>>>>>> Nick >> > > > > > >>>>>>>>> >> > > > > > >>>>>>>>> On Mon, 16 Oct 2023 at 14:24, Lucas Brutschy < >> > > > > > >>> lbruts...@confluent.io >> > > > > > >>>>>>>> .invalid> >> > > > > > >>>>>>>>> wrote: >> > > > > > >>>>>>>>> >> > > > > > >>>>>>>>>> Hi Nick! >> > > > > > >>>>>>>>>> >> > > > > > >>>>>>>>>> thanks for the KIP! I think this could be quite >> useful, >> > > given >> > > > > > the >> > > > > > >>>>>>>>>> problems that we had with leaks due to RocksDB >> resources >> > > not >> > > > > > >>> being >> > > > > > >>>>>>>>>> closed. >> > > > > > >>>>>>>>>> >> > > > > > >>>>>>>>>> I don't have any pressing issues why we can't accept >> it >> > > like it >> > > > > > >>> is, >> > > > > > >>>>>>>>>> just one minor point for discussion: would it also >> make >> > > sense to >> > > > > > >>>>>> make >> > > > > > >>>>>>>>>> it possible to identify a few very long-running / >> leaked >> > > > > > >>>>>> iterators? I >> > > > > > >>>>>>>>>> can imagine on a busy node, it would be hard to spot >> > that >> > > 1% of >> > > > > > >>> the >> > > > > > >>>>>>>>>> iterators never close when looking only at closed >> > > iterator or >> > > > > > the >> > > > > > >>>>>>>>>> number of iterators. But it could still be good to >> > > identify >> > > > > > those >> > > > > > >>>>>>>>>> leaks early. One option would be to add >> > > `iterator-duration-max` >> > > > > > >>> and >> > > > > > >>>>>>>>>> take open iterators into account when computing the >> > > metric. >> > > > > > >>>>>>>>>> >> > > > > > >>>>>>>>>> Cheers, >> > > > > > >>>>>>>>>> Lucas >> > > > > > >>>>>>>>>> >> > > > > > >>>>>>>>>> >> > > > > > >>>>>>>>>> On Thu, Oct 5, 2023 at 3:50 PM Nick Telford < >> > > > > > >>>>>> nick.telf...@gmail.com> >> > > > > > >>>>>>>>>> wrote: >> > > > > > >>>>>>>>>>> >> > > > > > >>>>>>>>>>> Hi Colt, >> > > > > > >>>>>>>>>>> >> > > > > > >>>>>>>>>>> I kept the details out of the KIP, because KIPs >> > generally >> > > > > > >>>>>> document >> > > > > > >>>>>>>>>>> high-level design, but the way I'm doing it is like >> > this: >> > > > > > >>>>>>>>>>> >> > > > > > >>>>>>>>>>> final ManagedKeyValueIterator<Bytes, >> byte[]> >> > > > > > >>>>>>>>>>> rocksDbPrefixSeekIterator = cf.prefixScan(accessor, >> > > > > > >>> prefixBytes); >> > > > > > >>>>>>>>>>> --> final long startedAt = System.nanoTime(); >> > > > > > >>>>>>>>>>> >> openIterators.add(rocksDbPrefixSeekIterator); >> > > > > > >>>>>>>>>>> rocksDbPrefixSeekIterator.onClose(() -> { >> > > > > > >>>>>>>>>>> --> >> > > > > > >>>>>>> >> metricsRecorder.recordIteratorDuration(System.nanoTime() >> > > > > > >>>>>>>> - >> > > > > > >>>>>>>>>>> startedAt); >> > > > > > >>>>>>>>>>> >> > > openIterators.remove(rocksDbPrefixSeekIterator); >> > > > > > >>>>>>>>>>> }); >> > > > > > >>>>>>>>>>> >> > > > > > >>>>>>>>>>> The lines with the arrow are the new code. This >> pattern >> > > is >> > > > > > >>>>>> repeated >> > > > > > >>>>>>>>>>> throughout RocksDBStore, wherever a new >> RocksDbIterator >> > > is >> > > > > > >>>>>> created. >> > > > > > >>>>>>>>>>> >> > > > > > >>>>>>>>>>> Regards, >> > > > > > >>>>>>>>>>> Nick >> > > > > > >>>>>>>>>>> >> > > > > > >>>>>>>>>>> On Thu, 5 Oct 2023 at 12:32, Colt McNealy < >> > > c...@littlehorse.io >> > > > > > > >> > > > > > >>>>>>>> wrote: >> > > > > > >>>>>>>>>>> >> > > > > > >>>>>>>>>>>> Thank you for the KIP, Nick! >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>>>> This would be highly useful for many reasons. Much >> > more >> > > sane >> > > > > > >>>>>> than >> > > > > > >>>>>>>>>> checking >> > > > > > >>>>>>>>>>>> for leaked iterators by profiling memory usage >> while >> > > running >> > > > > > >>>>>>> 100's >> > > > > > >>>>>>>> of >> > > > > > >>>>>>>>>>>> thousands of range scans via interactive queries (: >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>>>> One small question: >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>>>>> The iterator-duration metrics will be updated >> > whenever >> > > an >> > > > > > >>>>>>>> Iterator's >> > > > > > >>>>>>>>>>>> close() method is called >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>>>> Does the Iterator have its own "createdAt()" or >> > > equivalent >> > > > > > >>>>>> field, >> > > > > > >>>>>>>> or >> > > > > > >>>>>>>>>> do we >> > > > > > >>>>>>>>>>>> need to keep track of the Iterator's start time >> upon >> > > creation? >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>>>> Cheers, >> > > > > > >>>>>>>>>>>> Colt McNealy >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>>>> *Founder, LittleHorse.dev* >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>>>> On Wed, Oct 4, 2023 at 9:07 AM Nick Telford < >> > > > > > >>>>>>>> nick.telf...@gmail.com> >> > > > > > >>>>>>>>>>>> wrote: >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>>>>> Hi everyone, >> > > > > > >>>>>>>>>>>>> >> > > > > > >>>>>>>>>>>>> KIP-989 is a small Kafka Streams KIP to add a few >> new >> > > metrics >> > > > > > >>>>>>>> around >> > > > > > >>>>>>>>>> the >> > > > > > >>>>>>>>>>>>> creation and use of RocksDB Iterators, to aid >> users >> > in >> > > > > > >>>>>>>> identifying >> > > > > > >>>>>>>>>>>>> "Iterator leaks" that could cause applications to >> > leak >> > > native >> > > > > > >>>>>>>> memory. >> > > > > > >>>>>>>>>>>>> >> > > > > > >>>>>>>>>>>>> Let me know what you think! >> > > > > > >>>>>>>>>>>>> >> > > > > > >>>>>>>>>>>>> >> > > > > > >>>>>>>>>>>>> >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>> >> > > > > > >>>>>>>> >> > > > > > >>>>>>> >> > > > > > >>>>>> >> > > > > > >>>> >> > > > > > >>> >> > > > > > >> > > >> > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-989%3A+RocksDB+Iterator+Metrics >> > > > > > >>>>>>>>>>>>> >> > > > > > >>>>>>>>>>>>> P.S. I'm not too sure about the formatting of the >> > "New >> > > > > > >>>>>> Metrics" >> > > > > > >>>>>>>>>> table, >> > > > > > >>>>>>>>>>>> any >> > > > > > >>>>>>>>>>>>> advice there would be appreciated. >> > > > > > >>>>>>>>>>>>> >> > > > > > >>>>>>>>>>>>> Regards, >> > > > > > >>>>>>>>>>>>> Nick >> > > > > > >>>>>>>>>>>>> >> > > > > > >>>>>>>>>>>> >> > > > > > >>>>>>>>>> >> > > > > > >>>>>>>> >> > > > > > >>>>>>> >> > > > > > >>>>>> >> > > > > > >>>>> >> > > > > > >>>> >> > > > > > >>> >> > > > > > >> >> > > > > > > >> > > > > > >> > > >> > >> >
