KAFKA-10648: Add Prefix Scan support to State Stores by vamossagar12 · Pull Request #9508 · apache/kafka

vamossagar12 · 2020-10-27T05:18:06Z

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

cadonna

@vamossagar12, Thank you for the PR!

Here my feedback!

vamossagar12 · 2020-11-09T16:59:55Z

@vamossagar12, Thank you for the PR!

Here my feedback!

@cadonna thanks i made the requeiste changes barring 1.

vamossagar12 · 2020-11-10T03:10:22Z

@cadonna , The failing tests here https://github.com/apache/kafka/pull/9508/checks?check_run_id=1376108664 don't seem to be related to this PR. Would it be possible to retest?

vamossagar12 · 2020-11-15T15:32:08Z

hi @cadonna , wanted to know did you get a chance to look at my comment on the failing test case and the changes in general?

cadonna · 2020-11-16T09:06:39Z

@vamossagar12 I am really sorry. I haven't found the time yet, but it is on my ToDo list. I hope I will manage to make a pass this week.

cadonna

@vamossagar12 Thank you for the updates and sorry again for the long delay!

Here is my feedback!

cadonna · 2020-11-30T09:27:20Z

@vamossagar12 I replied to your comments. Let me know when the PR is ready for review.

vamossagar12 · 2020-12-01T13:11:17Z

Thanks @cadonna , I have resolved all comments. Plz review again if things look ok now..

vamossagar12 · 2020-12-11T09:39:49Z

hey @cadonna did you get a chance to look at the latest set of changes?

cadonna · 2020-12-11T10:05:58Z

@vamossagar12 I am really sorry, but I haven't found the time yet to review the changes. I hope I will find the time next week.

cadonna

Thanks @vamossagar12 for the updates!

I am wondering if we really need a separate prefix iterator or if we could just extend the range iterator and use that. See my comment in the respective iterator class.

vamossagar12 · 2021-01-13T13:03:02Z

@cadonna , made further changes. Plz review whenever you get a chance to do so!

cadonna

@vamossagar12, Thanks for the updates.

I have a couple of comments, but I also think that we are almost there.

vamossagar12 · 2021-01-20T12:30:38Z

@cadonna , done.

cadonna

Thanks for the updates, @vamossagar12 !

cadonna · 2021-01-26T10:11:07Z

    }

+    @Override
+    public <PS extends Serializer<P>, P> KeyValueIterator<Bytes, byte[]> prefixScan(final P prefix, final PS prefixKeySerializer) {


This method needs unit testing. Try to use a mock for the cache in the test.

Similar comment as below. Unit tests are in CachingInMemoryKeyValueStoreTest which already extends AbstractKeyValueStoreTest and creates an in memory cache store.

Oh, I see. I missed those. Sorry! That is fine then, although I think unit tests with mocks would be better.

Actually, I had created another ticket to streamline tests for CachingKVStore: https://issues.apache.org/jira/browse/KAFKA-10788. @rohitrmd had volunteered to take this up.

cadonna · 2021-01-26T10:35:35Z

    }

+    @Override
+    public <PS extends Serializer<P>, P> KeyValueIterator<Bytes, byte[]> prefixScan(final P prefix, final PS prefixKeySerializer) {


@vamossagar12 I can still not find the unit test for this method.

For this, do you want me to add the test cases here?https://github.com/apache/kafka/blob/17be91a37214bf77430c65d9300a5120e4348df9/streams/src/test/java/org/apache/kafka/streams/state/internals/InMemoryKeyValueStoreTest.java

There are tests in CachingInMemoryKeyValueStoreTest, which is where the tests for other methods like range etc have been added.

I think those tests never call the prefixScan() on the underlying in-memory state store because all entries fit into the cache. You would need to add another test that flushes the cache before you call prefixScan(). I would prefer a test that directly tests the in-memory store without any cache in between.

Here is the new ticket: https://issues.apache.org/jira/browse/KAFKA-12289 and the PR for the ticket:
#10052

cadonna · 2021-01-26T11:00:16Z

@vamossagar12 Could you please add a more descriptive commit message?

cadonna · 2021-01-26T11:01:28Z

Call for committer review: @mjsax @guozhangwang @ableegoldman

…/ChangeLoggingKeyValueBytesStoreTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

…/MeteredKeyValueStoreTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

…/metrics/StateStoreMetricsTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

guozhangwang

I made a quick pass on the PR and it looks good to me. After test is green I can merge it.

@cadonna could you also help @vamossagar12 to come up with a PR on docs for this KIP?

cadonna · 2021-02-02T10:56:46Z

@guozhangwang @vamossagar12 This failure is related to this PR:

org.apache.kafka.streams.state.internals.MeteredKeyValueStoreTest.shouldGetRecordsWithPrefixKey[0.10.0-2.4]

I provided the fix for this in the corresponding test.

cadonna · 2021-02-02T10:58:48Z

@vamossagar12 Probably, it would also be good to rebase this PR to current trunk to get a green build since we merged a couple of fixes to flaky tests recently.

cadonna · 2021-02-02T11:07:57Z

@cadonna could you also help @vamossagar12 to come up with a PR on docs for this KIP?

Yes, @vamossagar12, you should open a PR and add a short paragraph about KIP-614 in the upgrade guide. Looking forward to reviewing it! 🙂

…/MeteredKeyValueStoreTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

guozhangwang · 2021-02-03T18:26:56Z

I've merged this PR to trunk. Thanks @vamossagar12 so much for your contribution! And thanks to @cadonna for the very thorough review!

vamossagar12 · 2021-02-04T09:55:29Z

Thanks @guozhangwang !
@cadonna I have added the doc here in this PR: #10051.
I will create a separate. ticket for the missing test case for prefixScan. Thanks for your continued support @cadonna !

vamossagar12 force-pushed the kip-614 branch from d49a0af to 9bc1b4c Compare November 3, 2020 07:59

cadonna reviewed Nov 6, 2020

View reviewed changes

vamossagar12 force-pushed the kip-614 branch from 9bc1b4c to 899b797 Compare November 9, 2020 16:56

cadonna reviewed Nov 20, 2020

View reviewed changes

vamossagar12 force-pushed the kip-614 branch 2 times, most recently from 84b0ac8 to 0e45602 Compare December 1, 2020 13:04

vamossagar12 mentioned this pull request Dec 9, 2020

KAFKA-10766: Unit test cases for RocksDBRangeIterator #9717

Merged

cadonna reviewed Dec 17, 2020

View reviewed changes

mjsax mentioned this pull request Dec 22, 2020

Changes for Adding a new Prefix iterator confluentinc/kafka#242

Closed

3 tasks

vamossagar12 force-pushed the kip-614 branch 2 times, most recently from dd4c426 to 822edfe Compare January 13, 2021 13:01

cadonna reviewed Jan 15, 2021

View reviewed changes

KAFKA-10648: Add Prefix Scan support to State Stores

17be91a

vamossagar12 force-pushed the kip-614 branch from 98989a8 to 17be91a Compare January 20, 2021 12:28

cadonna reviewed Jan 26, 2021

View reviewed changes

vamossagar12 and others added 3 commits January 28, 2021 21:46

Update streams/src/test/java/org/apache/kafka/streams/state/internals…

4a41206

…/ChangeLoggingKeyValueBytesStoreTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

Update streams/src/test/java/org/apache/kafka/streams/state/internals…

33be911

…/ChangeLoggingKeyValueBytesStoreTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

Update streams/src/test/java/org/apache/kafka/streams/state/internals…

25980a0

…/ChangeLoggingKeyValueBytesStoreTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

vamossagar12 and others added 3 commits January 28, 2021 21:47

Update streams/src/test/java/org/apache/kafka/streams/state/internals…

a2ea513

…/ChangeLoggingKeyValueBytesStoreTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

Update streams/src/test/java/org/apache/kafka/streams/state/internals…

dddad17

…/MeteredKeyValueStoreTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

Update streams/src/test/java/org/apache/kafka/streams/state/internals…

d2479a4

…/metrics/StateStoreMetricsTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

guozhangwang approved these changes Jan 29, 2021

View reviewed changes

cadonna reviewed Feb 2, 2021

View reviewed changes

Comment thread streams/src/test/java/org/apache/kafka/streams/state/internals/MeteredKeyValueStoreTest.java Outdated

Update streams/src/test/java/org/apache/kafka/streams/state/internals…

8eca3c9

…/MeteredKeyValueStoreTest.java Co-authored-by: Bruno Cadonna <bruno@confluent.io>

guozhangwang merged commit 51833bf into apache:trunk Feb 3, 2021

Conversation

vamossagar12 commented Oct 27, 2020

Committer Checklist (excluded from commit message)

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vamossagar12 commented Nov 9, 2020

Uh oh!

vamossagar12 commented Nov 10, 2020

Uh oh!

vamossagar12 commented Nov 15, 2020

Uh oh!

cadonna commented Nov 16, 2020

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cadonna commented Nov 30, 2020

Uh oh!

vamossagar12 commented Dec 1, 2020

Uh oh!

vamossagar12 commented Dec 11, 2020

Uh oh!

cadonna commented Dec 11, 2020

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vamossagar12 commented Jan 13, 2021

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vamossagar12 commented Jan 20, 2021

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment