KAFKA-13211: add support for infinite range query for WindowStore by showuon · Pull Request #11227 · apache/kafka

showuon · 2021-08-18T09:09:22Z

Add support for infinite range query for WindowStore.
Story JIRA: https://issues.apache.org/jira/browse/KAFKA-13210

Committer Checklist (excluded from commit message)

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

showuon · 2021-08-18T09:12:14Z

The null cacheKeyFrom and cacheKeyTo will use range query, which is already supported in KIP-763.

showuon · 2021-08-18T09:15:30Z

side fix: the all case should be from == null && to == null. Otherwise, call range method, which is already supported null range query in KIP-763

Isn't it sufficient to distinguish the forward and reverse cases and just call range(from, to) or reverseRange(from, to)?

Good suggestion. Updated

showuon · 2021-08-20T13:47:06Z

@patrickstuedi @vvcephei , please take a look. Thank you.

patrickstuedi

Thanks for the work @showuon ! Just a few comments..

patrickstuedi · 2021-08-25T08:43:23Z

              files="StreamThread.java"/>

+    <suppress checks="BooleanExpressionComplexity"
+              files="InMemoryWindowStore.java"/>


I guess this is because of InMemoryWindowStore::isKeyWithinRange? Can we make that method more readable and at the same time avoid having to do this?

patrickstuedi · 2021-08-25T08:46:47Z

     * This iterator must be closed after use.
     *
     * @param keyFrom     the first key in the range
+     * A null value indicates a starting position from the first element in the store.


Can you make those extra lines have the same indentation than the previous line so it can easily be seen that they belong together?

patrickstuedi · 2021-08-25T09:18:10Z

            }
        }

+        private boolean isKeyWithinRange(final Bytes key) {


Per comment above, can you make this method more readable by splitting the statements?

patrickstuedi · 2021-08-25T09:23:03Z

-    public void shouldThrowNullPointerExceptionOnRangeNullToKey() {
-        assertThrows(NullPointerException.class, () -> windowStore.fetch(1, null, ofEpochMilli(1L), ofEpochMilli(2L)));
-    }
-


Instead of deleting maybe you want to change the name and check that the the store returns the right values.

Yes, but I've already tested these 2 test cases above (i.e. testFetchRange and testBackwardFetchRange). I don't think we should test them again here. What do you think?

Ok, no, if you have them covered up there that's fine.

patrickstuedi · 2021-08-25T09:27:24Z

+                iterator.next(),
+                new Windowed<>(bytesKey("a"), new TimeWindow(DEFAULT_TIMESTAMP, DEFAULT_TIMESTAMP + WINDOW_SIZE)),
+                "a");
+            assertFalse(iterator.hasNext());


Did you try if you can consolidate that code into a common method? It seem it's the same verification for different value sets in each of the tests.

Updated. Thanks.

patrickstuedi

Just a few more comments..

patrickstuedi · 2021-08-25T10:49:26Z

Isn't it sufficient to distinguish the forward and reverse cases and just call range(from, to) or reverseRange(from, to)?

showuon · 2021-08-26T07:34:39Z

@patrickstuedi , thanks for the comments. I've addressed all your comments and add test coverage. Please take a look again. Thank you.

patrickstuedi

Thanks for the update!

One more thing, it might be worth adding a topologyDriver based tests and an integration test, to test all combinations of layered window stores. You can take a look at the two tests for the non-window stores:
-streams/src/test/java/org/apache/kafka/streams/integration/KTableEfficientRangeQueryTest.java

streams/src/test/java/org/apache/kafka/streams/integration/RangeQueryIntegrationTest.java

showuon · 2021-09-02T12:34:55Z

Integration tests added, but found a bug that will fail these tests. Will wait for the PR got merged and continue this PR. Thanks. #11292

patrickstuedi · 2021-09-10T12:09:01Z

@showuon Any new updates on this?

showuon · 2021-09-10T12:18:26Z

@patrickstuedi , yes, the fix PR (#11292) is under reviewing (should be close). Thank you.

showuon · 2021-09-14T13:02:04Z

@patrickstuedi , the PR(#11292) is merged into trunk. I've rebased this PR, so it is good to review now. Thank you.

patrickstuedi

Thanks for the work @showuon! Looking good to me.

guozhangwang

Thanks @showuon the PR LGTM. The only meta comment I have in mind is the rationale to add a separate integration test on top of all the unit tests. Usually we have integration test in order to test the interaction between multiple modules, which would be more complicated (and more likely to become flaky due to timing issues), and takes more time to run. I feel that for this functionality just the augmented unit tests are sufficient, but I might be wrong so please let me know if you feel it does bring additional coverage.

guozhangwang · 2021-09-21T17:41:12Z

+            } else if (keyFrom == null && key.compareTo(getKey(keyTo)) <= 0) {
+                // start from the beginning
+                isKeyInRange = true;
+            } else if (key.compareTo(getKey(keyFrom)) >= 0 && keyTo == null) {


nit: let's move keyTo == null up first so that if it does not satisfy, we do not need to trigger getKey anymore.

Think about that a bit more, maybe we can make it simpler as:

if (keyFrom == null && keyTo == null) { // fetch all return true; } else if (keyFrom == null) { // start from the beginning return key.compareTo(getKey(keyTo)) <= 0; } else if (keyTo == null) { // end to the last return key.compareTo(getKey(keyFrom)) >= 0; } else { return key.compareTo(getKey(keyFrom)) >= 0 && key.compareTo(getKey(keyTo)) <= 0; }

Nice refactor! Thanks.

guozhangwang · 2021-09-21T17:47:53Z

+@SuppressWarnings("deprecation") // Old PAPI. Needs to be migrated.
+@RunWith(Parameterized.class)
+@Category({IntegrationTest.class})
+public class RangeQueryForWindowStoreIntegrationTest {


Could we refactor it into a unit test instead of an integration test? Does integration environment bring any additional coverage here?

Also, what additional coverage does this test provide on top of the augmented unit tests (which is great, btw!) below?

The reason we have an integration test here is because we use TopologyTestDriver in the unit test, without real brokers. But on second thought, I think we can remove the integration test, because that's why TopologyTestDriver exists, to simulate the streaming running. Especially this test case doesn't involve interaction between multiple modules. So I removed it now. Thank you.

guozhangwang · 2021-09-22T16:15:06Z

LGTM! Merged to trunk.

…ache#11227) Add support for infinite range query for WindowStore. Story JIRA: https://issues.apache.org/jira/browse/KAFKA-13210 Reviewers: Patrick Stuedi <pstuedi@gmail.com>, Guozhang Wang <wangguoz@gmail.com>

showuon commented Aug 18, 2021

View reviewed changes

Comment thread ...c/main/java/org/apache/kafka/streams/state/internals/AbstractRocksDBSegmentedBytesStore.java Outdated

showuon commented Aug 18, 2021

View reviewed changes

KAFKA-13211: add support for infinite range query for WindowStore

f270e18

showuon force-pushed the KAFKA-13211 branch from e87e905 to f270e18 Compare August 19, 2021 02:40

showuon changed the title ~~KAFKA-13211: add support for infinite range query for WindowStore~~ [WIP] KAFKA-13211: add support for infinite range query for WindowStore Aug 19, 2021

showuon changed the title ~~[WIP] KAFKA-13211: add support for infinite range query for WindowStore~~ KAFKA-13211: add support for infinite range query for WindowStore Aug 20, 2021

patrickstuedi reviewed Aug 25, 2021

View reviewed changes

showuon added 2 commits August 25, 2021 20:33

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-13211

5cb3685

KAFKA-13211: refactor codes

55551f3

showuon force-pushed the KAFKA-13211 branch from c73bddf to 55551f3 Compare August 25, 2021 13:37

KAFKA-13211: fix bug and improve test coverage

88cc410

patrickstuedi reviewed Aug 26, 2021

View reviewed changes

showuon added 2 commits September 2, 2021 11:18

KAFKA-13211: add integration tests

688c422

KAFKA-13264: fix inMemoryStateStore backward fetch not in reversed order

db0c299

patrickstuedi mentioned this pull request Sep 10, 2021

feat: adding range query optimization for windowed tabled confluentinc/ksql#8116

Closed

2 tasks

showuon added 2 commits September 14, 2021 20:40

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-13211

4dccb30

KAFKA-13211: fix inMemory state store backward fetch issue

eb19d12

showuon force-pushed the KAFKA-13211 branch from e8d013f to eb19d12 Compare September 14, 2021 13:00

patrickstuedi approved these changes Sep 21, 2021

View reviewed changes

guozhangwang reviewed Sep 21, 2021

View reviewed changes

KAFKA-13211: refactor

d3cee05

guozhangwang merged commit b61ec00 into apache:trunk Sep 22, 2021

Conversation

showuon commented Aug 18, 2021

Committer Checklist (excluded from commit message)

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon commented Aug 20, 2021

Uh oh!

patrickstuedi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickstuedi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

showuon commented Aug 26, 2021

Uh oh!

patrickstuedi left a comment

Choose a reason for hiding this comment

Uh oh!

showuon commented Sep 2, 2021

Uh oh!

patrickstuedi commented Sep 10, 2021

Uh oh!

showuon commented Sep 10, 2021

Uh oh!

showuon commented Sep 14, 2021

Uh oh!

patrickstuedi left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Sep 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

patrickstuedi left a comment •

edited

Loading