KAFKA-20219: Headers store with caching enabled should reject queries#21941
KAFKA-20219: Headers store with caching enabled should reject queries#21941mjsax merged 5 commits intoapache:trunkfrom
Conversation
github-actions
Bot
added
triage
zheguang
changed the title
zheguang
changed the title
mjsax
left a comment
mjsax
left a comment
There was a problem hiding this comment.
Thanks for the PR. Overall LGTM.
But it seems, we are missing a test for IQv2 that verifies that we indeed get an error-result back for kv-header-store case with caching enanbled?
| final File dir = TestUtils.tempDirectory(); | ||
| final Properties props = StreamsTestUtils.getStreamsConfig(); | ||
| final InternalMockProcessorContext<String, String> context = new InternalMockProcessorContext<>( | ||
| final InternalMockProcessorContext<String, String> context = spy(new InternalMockProcessorContext<>( |
There was a problem hiding this comment.
Would it be easier to just pass in the ThreadCache into the constructor of InternalMockProcessorContext instead of using a spy?
There was a problem hiding this comment.
Indeed. Done. Thanks for the suggestion! :)
Co-authored-by: Matthias J. Sax <mjsax@apache.org>
| @ParameterizedTest | ||
| @ValueSource(booleans = {true, false}) | ||
| public void shouldReturnUnknownQueryTypeForKeyQueryOnHeadersStore(final boolean cachingEnabled) { | ||
| final TimestampedKeyValueStoreWithHeaders<String, String> store = headersStoreMaybeWithCache(cachingEnabled); |
There was a problem hiding this comment.
missing a test for IQv2 that verifies that we indeed get an error-result back for kv-header-store case with caching enanbled
This is covered by this test when cachingEnabled = true... right?
There was a problem hiding this comment.
Actually, maybe you meant IQv2StoreIntegrationTest... let me look into adding an integration test there
|
Hm... the test failure |
Not sure what you exactly mean by this? Jenkins is green. Did also re-run IQv2StoreIntegrationTest on trunk plus this branch rebased on trunk, and both passed. |
…#21941) For headers-store with caching enabled, we want to keep IQv2 disabled until we implement IQv2 across the board. Thus, we need to introduce CachingKeyValueStoreWithHeaders and just forward any query to the underlying store, instead of re-using the existing query implementation on CachingKeyValueStore. Reviewers: Matthias J. Sax <matthias@confluent.io> --------- Co-authored-by: Matthias J. Sax <mjsax@apache.org>
|
Thanks for the PR. Merged to |
Never mind, just a test failure I saw before I merged trunk. Thanks for the review! |
…apache#21941) For headers-store with caching enabled, we want to keep IQv2 disabled until we implement IQv2 across the board. Thus, we need to introduce CachingKeyValueStoreWithHeaders and just forward any query to the underlying store, instead of re-using the existing query implementation on CachingKeyValueStore. Reviewers: Matthias J. Sax <matthias@confluent.io> --------- Co-authored-by: Matthias J. Sax <mjsax@apache.org>
2 participants
For headers-store with caching enabled, we want to keep IQv2 disabled until we implement IQv2 across the board. Thus, we need to introduce CachingKeyValueStoreWithHeaders and just forward any query to the underlying store, instead of re-using the existing query implementation on CachingKeyValueStore.
Reviewers: Matthias J. Sax matthias@confluent.io