MINOR: De-duplicate the metering lifecycle across Metered*WithHeaders iterators - #22975
Merged
bbejeck merged 7 commits intoAug 11, 2026
Merged
Conversation
aliehsaeedii
reviewed
Aug 4, 2026
…path tests Broaden the de-duplication per review: rework the shared base into a lifecycle-only AbstractMeteredIterator<RawKey> implements MeteredIterator (no longer binds K/V or a result interface), delete the duplicate inner AbstractMeteredIterator in MeteredTimestampedKeyValueStoreWithHeaders, and have all seven Metered*WithHeaders iterators extend it while declaring their own result interface (ReadOnlyRecordIterator or KeyValueIterator). Add close-path iterator-duration tests: one per test class exercising the ReadOnlyRecordIterator via its query, plus a shouldTimeIteratorDuration for the window store's KeyValueIterator sibling (which had no duration coverage), each asserting the operation and iterator-duration sensors record on close.
Jess668
force-pushed
the
dedup-metered-withheaders-readonly-iterators
branch
from
August 5, 2026 14:19
be2723b to
151d3a7
Compare
aliehsaeedii
reviewed
Aug 6, 2026
| * | ||
| * @param <RawKey> the raw iterator's key type | ||
| */ | ||
| abstract class AbstractMeteredIterator<RawKey> implements MeteredIterator { |
Contributor
There was a problem hiding this comment.
MeteredWindowedKeyValueIterator, MeteredWindowStoreIterator and MeteredKeyValueStoreIterator still hand-roll this exact lifecycle, field for field. The first is the base of MeteredWindowedKeyValueWithHeadersIterator, so one Metered*WithHeaders iterator is still left out. Should we do a follow-up making them extend this class, after which the javadoc's Metered*WithHeaders scoping can go.
Contributor
Author
There was a problem hiding this comment.
agreed, will do the migration in a follow up to keep this PR scoped to the *WithHeaders extraction
…tion tests - Make AbstractMeteredIterator.startTimestamp() final: the constructor's openIterators.add(this) sorts through it via the set's startTimestamp comparator, on a half-built object, so a subclass must not be able to override it with something that reads its own not-yet-assigned state. - Tighten the new iterator-duration tests: open two iterators (2ms then 3ms) and assert exact avg (2.5ms) and max (3ms) instead of one sample / > 0.0, so avg is actually pinned distinctly from max. Applied across the KV, session and window ReadOnlyRecord tests and the window KeyValueIterator sibling test.
…ation Bring the session store's sibling-path duration test in line with the KV one (already two-sample) and the tests added earlier in this PR: open two iterators (2ms then 3ms) and assert exact avg (2.5ms) and max (3ms) instead of a single sample / > 0.0, so avg is pinned distinctly from max.
aliehsaeedii
reviewed
Aug 6, 2026
…lose() - Assert the operation sensor's latency (get-latency-avg / fetch-latency-avg) is exactly 2.5ms in the three ReadOnlyRecord duration tests instead of > 0.0: that sensor is recorded only from the iterator's close() on these paths, so the two samples (2ms, 3ms) average deterministically. - Make AbstractMeteredIterator.close() final, like startTimestamp(): it owns the metering lifecycle (sensor recording, numOpenIterators decrement, openIterators deregistration), so a subclass overriding it and forgetting super.close() would silently drop the decrement and deregistration.
2 tasks
aliehsaeedii
approved these changes
Aug 11, 2026
Jess668
changed the title
bbejeck
approved these changes
Aug 11, 2026
bbejeck
pushed a commit
that referenced
this pull request
Aug 11, 2026
… iterators (#22975) The metered iterators backing the headers-aware IQv2 query types and the `Metered*WithHeaders` stores' own range/fetch/find methods each hand-rolled the same metering lifecycle: stamping the open time (for `oldest-iterator-open-since-ms`), registering in `numOpenIterators`/`openIterators`, and recording the operation and iterator-duration sensors on `close()`. Only `next()` (and, for the `KeyValueIterator` ones, `peekNextKey()`/`hasNext()`) genuinely differs per store. Extract that lifecycle into a shared, result-type-agnostic `AbstractMeteredIterator<RawKey>` (implements `MeteredIterator`). All seven `Metered*WithHeaders` iterators now extend it and declare their own result interface: - three `ReadOnlyRecordIterator` iterators (the session / timestamped-window / timestamped-key-value headers query results), and - four `KeyValueIterator` iterators (the session and timestamped-window `KeyValueIterator` siblings, plus the timestamped-key-value query and plain iterators). This also folds away the former per-file `AbstractMeteredIterator` inside `MeteredTimestampedKeyValueStoreWithHeaders`, so there is a single metering base. `startTimestamp()` and `close()` are `final`: the constructor registers `this` in a `startTimestamp`-ordered set (so a subclass must not override `startTimestamp()` with not-yet-assigned state), and `close()` owns the sensor recording, `numOpenIterators` decrement and `openIterators` deregistration (so a subclass must not silently drop them). Adds iterator-duration close-path tests across the three stores (two samples each, pinning avg vs max and the operation sensor exactly), including the window store's first duration coverage. Follow-ups (out of scope, tracked separately): migrating the non-headers metered iterators (`MeteredWindowedKeyValueIterator`, `MeteredWindowStoreIterator`, `MeteredKeyValueStoreIterator`) onto this base; KAFKA-20902 (oldest-iterator metric same-millisecond collision); KAFKA-20922 (TimestampedRangeWithHeadersQuery iterator can NPE on a null-deserialized value). Reviewers: Alieh Saeedi <asaeedi@confluent.io>, Suman Pal (github:sumanpal97), Bill Bejeck <bbejeck@apache.org>
Member
|
Merged #22975 into trunk |
Member
|
cherry-picked to 4.4 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The metered iterators backing the headers-aware IQv2 query types and the
Metered*WithHeadersstores' own range/fetch/find methods eachhand-rolled the same metering lifecycle: stamping the open time (for
oldest-iterator-open-since-ms), registering innumOpenIterators/openIterators, and recording the operation anditerator-duration sensors on
close(). Onlynext()(and, for theKeyValueIteratorones,peekNextKey()/hasNext()) genuinely differsper store.
Extract that lifecycle into a shared, result-type-agnostic
AbstractMeteredIterator<RawKey>(implementsMeteredIterator). Allseven
Metered*WithHeadersiterators now extend it and declare theirown result interface:
ReadOnlyRecordIteratoriterators (the session /timestamped-window /
timestamped-key-value headers query results), and
KeyValueIteratoriterators (the session and timestamped-windowKeyValueIteratorsiblings, plus the timestamped-key-value query and plain iterators).
This also folds away the former per-file
AbstractMeteredIteratorinside
MeteredTimestampedKeyValueStoreWithHeaders, so there is asingle metering base.
startTimestamp()andclose()arefinal: theconstructor registers
thisin astartTimestamp-ordered set (so asubclass must not override
startTimestamp()with not-yet-assignedstate), and
close()owns the sensor recording,numOpenIteratorsdecrement and
openIteratorsderegistration (so a subclass must notsilently drop them).
Adds iterator-duration close-path tests across the three stores (two
samples each, pinning avg vs max and the operation sensor exactly),
including the window store's first duration coverage.
Follow-ups (out of scope, tracked separately): migrating the non-headers
metered iterators (
MeteredWindowedKeyValueIterator,MeteredWindowStoreIterator,MeteredKeyValueStoreIterator) onto thisbase; KAFKA-20902 (oldest-iterator metric same-millisecond collision);
KAFKA-20922 (TimestampedRangeWithHeadersQuery iterator can NPE on a
null-deserialized value).
Reviewers: Alieh Saeedi asaeedi@confluent.io, Suman Pal
(github:sumanpal97), Bill Bejeck bbejeck@apache.org