KAFKA-19398: (De)Register oldest-iterator-open-since-ms metric dynamically by mjsax · Pull Request #20022 · apache/kafka

mjsax · 2025-06-23T17:56:10Z

The metric for oldest-iterator-open-since-ms might report a null value
if there is not open iterator.

This PR changes the behavior to dynamically register/deregister the
entire metric instead of allowing it to return a null value.

Reviewers: Bill Bejeck bbejeck@apache.org

…cally The metric for oldest-iterator-open-since-ms might report a null value if there is not open iterator. This PR changes the behavior to dynamically register/deregister the entire metric instead of allowing it to return a null value.

bbejeck

Thanks for the PR @mjsax overall looks good - just a couple of minor comments

bbejeck · 2025-06-24T22:09:11Z

+
+    public void add(final MeteredIterator iterator) {
+        openIterators.add(iterator);
+        numOpenIterators.increment();


Why not openIterators.size()?

Seems this has historic reasons -- I just "refactored" existing code.

KIP-989 introduced 4 new metrics which got added by 4 PRs.

numOpenIterators -> KAFKA-15541: Add num-open-iterators metric #15975

openIterators -> KAFKA-15541: Add oldest-iterator-open-since-ms metric #16041
(omitting the other two PRs as they are irrelevant)

There was also a follow up PR fro numOpenIterators: #16076

@nicktelford do you see any reason why we need both variables, or can we unify them?

Guess we can also do this unification only in trunk to get this into 4.1 on time?

Guess we can also do this unification only in trunk to get this into 4.1 on time?

That works for me

I'm not certain, but it's possible the LongAdder and Set duplicating the counting functionality is just an oversight. In theory, using the LongAdder would yield improved performance vs. openIterators.size(), however, in practice the total number of open iterators is not likely to be large enough to be a problem, so it seems reasonable to remove numOpenIterators.

Are you interested to do a PR for this @nicktelford? If not, happy to do one myself.

Just did a quick PR for it: #20060

bbejeck · 2025-06-24T22:26:06Z

            storeLevelMetrics.computeIfAbsent(key, ignored -> new LinkedList<>()).push(metricName);
        }
+
+        return metricName;


Why does addX return the MetricName obejct?

We need the MetricName to be able to remove it when the iterator count goes to zero.

bbejeck · 2025-06-24T22:29:40Z

+                                                        final String storeName,
+                                                        final StreamsMetricsImpl streamsMetrics,
+                                                        final Gauge<Long> oldestOpenIteratorGauge) {
+        return streamsMetrics.addStoreLevelMutableMetric(


Same question here

Same answer :)

mjsax · 2025-06-24T23:39:41Z

+
+    public void add(final MeteredIterator iterator) {
+        openIterators.add(iterator);
+        numOpenIterators.increment();


Seems this has historic reasons -- I just "refactored" existing code.

KIP-989 introduced 4 new metrics which got added by 4 PRs.

numOpenIterators -> KAFKA-15541: Add num-open-iterators metric #15975

openIterators -> KAFKA-15541: Add oldest-iterator-open-since-ms metric #16041
(omitting the other two PRs as they are irrelevant)

There was also a follow up PR fro numOpenIterators: #16076

@nicktelford do you see any reason why we need both variables, or can we unify them?

mjsax · 2025-06-24T23:40:27Z

            storeLevelMetrics.computeIfAbsent(key, ignored -> new LinkedList<>()).push(metricName);
        }
+
+        return metricName;


We need the MetricName to be able to remove it when the iterator count goes to zero.

mjsax · 2025-06-24T23:40:48Z

+                                                        final String storeName,
+                                                        final StreamsMetricsImpl streamsMetrics,
+                                                        final Gauge<Long> oldestOpenIteratorGauge) {
+        return streamsMetrics.addStoreLevelMutableMetric(


Same answer :)

mjsax · 2025-06-24T23:41:51Z

+        numOpenIterators.increment();
+
+        if (numOpenIterators.intValue() == 1) {
+            metricName = StateStoreMetrics.addOldestOpenIteratorGauge(taskId.toString(), metricsScope, name, streamsMetrics,


Answer to your question: we need to remember the MetricName here, to be able to use it to remove the metric if the iterator count goes back to zero.

bbejeck

LGTM

mjsax · 2025-06-25T00:11:21Z

Merged to trunk and cherry-picked to 4.1 branch.

…cally (#20022) The metric for oldest-iterator-open-since-ms might report a null value if there is not open iterator. This PR changes the behavior to dynamically register/deregister the entire metric instead of allowing it to return a null value. Reviewers: Bill Bejeck <bbejeck@apache.org>

…cally (apache#20022) The metric for oldest-iterator-open-since-ms might report a null value if there is not open iterator. This PR changes the behavior to dynamically register/deregister the entire metric instead of allowing it to return a null value. Reviewers: Bill Bejeck <bbejeck@apache.org>

…c dynamically (apache#20022)" This reverts commit fb054b5.

mjsax added the streams label Jun 23, 2025

mjsax force-pushed the kafka-19398-iterator-metric branch from 71192dc to 05e6afd Compare June 23, 2025 18:00

mjsax added 2 commits June 23, 2025 11:00

minor

323ba43

fix

4da2d8f

bbejeck reviewed Jun 24, 2025

View reviewed changes

mjsax commented Jun 24, 2025

View reviewed changes

bbejeck approved these changes Jun 24, 2025

View reviewed changes

mjsax merged commit 4387132 into apache:trunk Jun 25, 2025
28 checks passed

mjsax deleted the kafka-19398-iterator-metric branch June 25, 2025 00:02

mjsax mentioned this pull request Jun 27, 2025

MINOR: simplify OpenIterator #20060

Merged

lucasbru added a commit to lucasbru/kafka that referenced this pull request Jul 14, 2025

Revert "KAFKA-19398: (De)Register oldest-iterator-open-since-ms metri…

c45fb2f

…c dynamically (apache#20022)" This reverts commit fb054b5.

Nikita-Shupletsov mentioned this pull request Sep 9, 2025

KAFKA-19679: Fix NoSuchElementException in oldest open iterator metric #20512

Merged

Conversation

mjsax commented Jun 23, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mjsax commented Jun 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mjsax commented Jun 23, 2025 •

edited by github-actions bot

Loading

mjsax Jun 24, 2025 •

edited

Loading

mjsax Jun 24, 2025 •

edited

Loading