Skip to content

KAFKA-20497: Add TimeOrderedCachingWindowStore#readOnly#22315

Open
nicktelford wants to merge 3 commits into
apache:trunkfrom
nicktelford:KIP-892/iq-isolation-caching-window-timeordered
Open

KAFKA-20497: Add TimeOrderedCachingWindowStore#readOnly#22315
nicktelford wants to merge 3 commits into
apache:trunkfrom
nicktelford:KIP-892/iq-isolation-caching-window-timeordered

Conversation

@nicktelford

@nicktelford nicktelford commented May 18, 2026

Copy link
Copy Markdown
Contributor

The cache holds uncommitted writes that must not be visible under
READ_COMMITTED, so that isolation level bypasses the cache entirely and
delegates straight to the inner store's readOnly view. READ_UNCOMMITTED
requires a merged view through a ReadOnlyView. The ReadOnlyView
delegates to the existing fetchInternal/fetchKeyRange/fetchAllInternal
helpers and converts Instant arguments to epoch-milliseconds before
calling them; the cache key schema operates in longs throughout, so
converting early avoids repeated Instant-to-long conversions in iterator
hot paths.

KAFKA-20497

Reviewers: Bill Bejeck bbejeck@apache.org

@nicktelford

Copy link
Copy Markdown
Contributor Author

@bbejeck

@github-actions github-actions Bot added triage PRs from the community streams labels May 18, 2026
@github-actions

Copy link
Copy Markdown

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

@nicktelford nicktelford force-pushed the KIP-892/iq-isolation-caching-window-timeordered branch from 3ba32a2 to 81dc06a Compare June 1, 2026 18:08

@bbejeck bbejeck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nicktelford - overall looks good but there are a couple of failing tests

  1. org.apache.kafka.streams.state.internals.TimeOrderedCachingPersistentWindowStoreTest#shouldThrowOnNullInstantInViewFetch
  2. org.apache.kafka.streams.state.internals.TimeOrderedCachingPersistentWindowStoreTest#shouldNotThrowInvalidBackwardRangeExceptionWithNegativeFromKey

@github-actions github-actions Bot removed needs-attention triage PRs from the community labels Jun 2, 2026
@nicktelford nicktelford force-pushed the KIP-892/iq-isolation-caching-window-timeordered branch from 81dc06a to a365582 Compare June 2, 2026 11:43
@nicktelford

Copy link
Copy Markdown
Contributor Author

Thanks @nicktelford - overall looks good but there are a couple of failing tests

  1. org.apache.kafka.streams.state.internals.TimeOrderedCachingPersistentWindowStoreTest#shouldThrowOnNullInstantInViewFetch
  2. org.apache.kafka.streams.state.internals.TimeOrderedCachingPersistentWindowStoreTest#shouldNotThrowInvalidBackwardRangeExceptionWithNegativeFromKey

@bbejeck Rebased on trunk; these test failures should now be resolved.

@bbejeck

bbejeck commented Jun 2, 2026

Copy link
Copy Markdown
Member

@nicktelford - needs ./gradlew spotlessApply

@bbejeck

bbejeck commented Jun 3, 2026

Copy link
Copy Markdown
Member

Thanks @nicktelford - looks good overall but there are some test failures which I think can be resolved by updating the log statement on line 526 is in TimeOrderedCachingWindowStore

nicktelford and others added 3 commits June 4, 2026 16:20
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The warning logged when a key range fetch has `from > to` only mentioned
serde ordering issues as a possible cause, omitting the more obvious case
where the caller simply passes the range arguments in the wrong order.
Adding that phrase gives users a clearer first-line diagnostic before
they investigate their serde implementations.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@nicktelford nicktelford force-pushed the KIP-892/iq-isolation-caching-window-timeordered branch from ce95136 to 3710346 Compare June 4, 2026 15:29
@nicktelford

Copy link
Copy Markdown
Contributor Author

@bbejeck I think I fixed this the other day but forgot to push the new commit 🤦‍♂️
Also rebased on trunk.

@bbejeck

bbejeck commented Jun 5, 2026

Copy link
Copy Markdown
Member

Thanks @nicktelford - still getting the same test failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants