KAFKA-15541: Add num-open-iterators metric#15975
Conversation
Part of [KIP-989](https://cwiki.apache.org/confluence/x/9KCzDw). This new `StateStore` metric tracks the number of `Iterator` instances that have been created, but not yet closed (via `AutoCloseable#close()`). This will aid users in detecting leaked iterators, which can cause major performance problems.
|
@mjsax @lucasbru @ableegoldman I know the vote hasn't closed yet, but I thought I'd get a head-start on the review. I'm submitting each metric from KIP-989 as a separate PR, to aid review, and because they can be merged independently. I think I've covered every Metered Iterator created; I searched for Note: I'm not completely sold on the test strategy here. I'm currently calling one Iterator-creating method on each Metered store type (e.g. |
Like all the other Iterators, we need to ensure we decrement our counter even if an exception is thrown while closing the underlying Iterator.
lucasbru
left a comment
lucasbru
left a comment
There was a problem hiding this comment.
Others know the state store hierarchy better than I do, so maybe it would be good to get another review. But I couldn't find anything wrong with this change, so LGTM, thanks!
| flushSensor = StateStoreMetrics.flushSensor(taskId.toString(), metricsScope, name(), streamsMetrics); | ||
| deleteSensor = StateStoreMetrics.deleteSensor(taskId.toString(), metricsScope, name(), streamsMetrics); | ||
| e2eLatencySensor = StateStoreMetrics.e2ELatencySensor(taskId.toString(), metricsScope, name(), streamsMetrics); | ||
| StateStoreMetrics.addNumOpenIteratorsGauge(taskId.toString(), metricsScope, name(), streamsMetrics, |
There was a problem hiding this comment.
Would it be better to add a Sensor that allows us to track the different metrics in one go?
There was a problem hiding this comment.
Can Sensors track Gauges? I don't believe that they can.
…/metrics/StateStoreMetrics.java Use lowercase for "iterators" in description Co-authored-by: Matthias J. Sax <mjsax@apache.org>
|
Merged to About testing: I agree that we might not need to test every method which creates an iterator, but it would be great to test one method per "iterator type", per store? Ie, if there are two constructors for two iterator classed, we should try to test both by having a test for one method which create the one, or other iterator? About Sensor: you are right, that it currently does not support |
@mjsax Since |
|
Makes sense. Could you file a Jira for the KIP and the follow up cleanup for KS code to use it :) |
Part of [KIP-989](https://cwiki.apache.org/confluence/x/9KCzDw). This new `StateStore` metric tracks the number of `Iterator` instances that have been created, but not yet closed (via `AutoCloseable#close()`). This will aid users in detecting leaked iterators, which can cause major performance problems. Reviewers: Lucas Brutschy <lbrutschy@confluent.io>, Matthias J. Sax <matthias@confluent.io>
Part of [KIP-989](https://cwiki.apache.org/confluence/x/9KCzDw). This new `StateStore` metric tracks the number of `Iterator` instances that have been created, but not yet closed (via `AutoCloseable#close()`). This will aid users in detecting leaked iterators, which can cause major performance problems. Reviewers: Lucas Brutschy <lbrutschy@confluent.io>, Matthias J. Sax <matthias@confluent.io>
Part of [KIP-989](https://cwiki.apache.org/confluence/x/9KCzDw). This new `StateStore` metric tracks the number of `Iterator` instances that have been created, but not yet closed (via `AutoCloseable#close()`). This will aid users in detecting leaked iterators, which can cause major performance problems. Reviewers: Lucas Brutschy <lbrutschy@confluent.io>, Matthias J. Sax <matthias@confluent.io>
Part of [KIP-989](https://cwiki.apache.org/confluence/x/9KCzDw). This new `StateStore` metric tracks the number of `Iterator` instances that have been created, but not yet closed (via `AutoCloseable#close()`). This will aid users in detecting leaked iterators, which can cause major performance problems. Reviewers: Lucas Brutschy <lbrutschy@confluent.io>, Matthias J. Sax <matthias@confluent.io>
3 participants
Part of KIP-989.
This new
StateStoremetric tracks the number ofIteratorinstances that have been created, but not yet closed (viaAutoCloseable#close()).This will aid users in detecting leaked iterators, which can cause major performance problems.