-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@@ -162,6 +165,8 @@ private void registerMetrics() { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can Sensor
s track Gauge
s? I don't believe that they can.
streams/src/main/java/org/apache/kafka/streams/state/internals/metrics/StateStoreMetrics.java
Outdated
Show resolved
Hide resolved
…/metrics/StateStoreMetrics.java Use lowercase for "iterators" in description Co-authored-by: Matthias J. Sax <[email protected]>
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 <[email protected]>, Matthias J. Sax <[email protected]>
Part of KIP-989.
This new
StateStore
metric tracks the number ofIterator
instances that have been created, but not yet closed (viaAutoCloseable#close()
).This will aid users in detecting leaked iterators, which can cause major performance problems.