MINOR: De-duplicate Metered*WithHeaders read-only-record iterators - #22975
Open
Jess668 wants to merge 5 commits into
Open
MINOR: De-duplicate Metered*WithHeaders read-only-record iterators#22975Jess668 wants to merge 5 commits into
Jess668 wants to merge 5 commits into
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
| operationSensor.record(duration); | ||
| iteratorSensor.record(duration); | ||
| numOpenIterators.decrement(); | ||
| openIterators.remove(this); |
Contributor
There was a problem hiding this comment.
openIterators is a ConcurrentSkipListSet keyed only by startTimestamp(), so two iterators opened in the same millisecond collide: the second add is silently dropped and this remove can evict the other, still-open one. Not this PR's to fix, but worth a ticket.
Contributor
Author
There was a problem hiding this comment.
…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.
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
2 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 read-only-record iterators backing the headers-aware IQv2 query types (
MeteredSessionStoreWithHeaders,MeteredTimestampedWindowStoreWithHeaders,MeteredTimestampedKeyValueStoreWithHeaders) each hand-rolled the same metering lifecycle: stamping the open time, registering in numOpenIterators/openIterators, and recording the operation and iterator-duration sensors on close. Onlynext()genuinely differs per store.Extract that lifecycle into a shared
AbstractMeteredReadOnlyRecordIterator<RawKey, K, V>. Each store's iterator now extends it and implements onlynext().Reviewers: Alieh Saeedi asaeedi@confluent.io