[MINOR] Guard against crashing on invalid key range queries by ableegoldman · Pull Request #6521 · apache/kafka

ableegoldman · 2019-03-29T00:46:39Z

Due to KAFKA-8159, Streams will throw an unchecked exception when a caching layer or in-memory underlying store is queried over a range of keys from negative to positive. We should add a check for this and log it then return an empty iterator (as the RocksDB stores happen to do) rather than crash

Committer Checklist (excluded from commit message)

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

ableegoldman · 2019-03-29T00:49:45Z

@guozhangwang @bbejeck @vvcephei @mjsax

ableegoldman · 2019-03-29T18:25:46Z

Streams should at least be consistent across store types in its handling of invalid range queries, and I felt it was better to log the error and return nothing than to throw an exception. However maybe silently returning "incorrect" results is worse than crashing and alerting users to the issue...WDYT?

guozhangwang

One meta comment: should we add documentations similar to https://github.com/apache/flink/blob/master/flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBCachingPriorityQueueSet.java#L71 to indicate the object logical ordering and the serialized lexicographic ordering to be consistent as well?

ableegoldman · 2019-03-29T19:40:52Z

This ordering only needs to be enforced for IQ, correct?

guozhangwang · 2019-04-04T20:45:41Z

This ordering only needs to be enforced for IQ, correct?

I think it should be applied universally, since whenever you call a put, which will serialize the object to bytes, ordering happened already -- if the serialized bytes are put not accordingly to the object order, then a follow-up range-fetch, either from IQ or from the processor, will return wrong result.

bbejeck

Thanks @ableegoldman. Overall looks good to me I just have a minor comment regarding the logging level.

bbejeck · 2019-04-04T21:05:21Z

nit: since this represents an invalid range maybe this could be a WARN?

ack, good point

bbejeck · 2019-04-04T21:05:37Z

bbejeck · 2019-04-04T21:05:51Z

ditto here and below

bbejeck

Thanks for the update @ableegoldman LGTM.

bbejeck · 2019-04-05T14:02:01Z

call for second review any of @guozhangwang @mjsax @vvcephei @cadonna

cadonna

Hi @ableegoldman,

Are there unit tests in place to verify the changes in this PR?

For the rest, I have just a couple of nits.

cadonna · 2019-04-05T18:16:21Z

    @Override
    public KeyValueIterator<Bytes, byte[]> range(final Bytes from,
                                                 final Bytes to) {
+        // Make sure this is a valid query


nit: I would remove the comment here (and in all occurrences below), because the code itself is clear enough about what it does. Maybe rename from and to to fromKey and toKey (or similar) to make it even more clearer. Renaming would also apply to some of the changes below.

cadonna · 2019-04-05T18:38:50Z

                                                 final Bytes to) {
+        // Make sure this is a valid query
+        if (from.compareTo(to) > 0) {
+            LOG.warn("Returning empty iterator for range query with invalid range: keyFrom > keyTo.");


nit: I would avoid to write variable names (i.e., keyFrom and keyTo) to a log, because they are hard to maintain consistently with the code (as you can see here).

…ror messages

bbejeck · 2019-04-05T21:11:44Z

+
+    @Test
+    public void shouldNotThrowInvalidRangeExceptionWithNegativeFromKey() {
+        store.range(-1, 1);


You can use org.apache.kafka.streams.processor.internals.testutil.LogCaptureAppender to assert the correct log message

Ah thanks, will add to tests

ableegoldman · 2019-04-08T19:01:55Z

retest this, please

cadonna · 2019-04-08T22:07:47Z

+        LogCaptureAppender.setClassLoggerToDebug(InMemoryWindowStore.class);
+        final LogCaptureAppender appender = LogCaptureAppender.createAndRegister();
+
+        store.range(-1, 1);


Could you add a check to verify that the returned iterator is empty. Something along the lines of assertThat(iterator.hasNext(), is(false))?

Could you also add a test for a range query where the start key is equal to the end key? Such a unit test ensures correct behaviour for this special case.

nit: I would rename the test to shouldReturnEmptyIteratorForRangeQueryWithInvalidKeyRange. Correct me, if I am wrong, but I think the empty iterator and the invalid key range are the points here, not the negative starting key. I would even change the range from (-1, 1) to (5, 3). It took me a bit to understand why (-1, 1) is an invalid range.

These comments apply also to the unit tests below.

Ack Re: verify returned iterator is empty, add unit tests for equal start/end keys

Regarding your third point, this patch is mostly aimed at the bug in [https://issues.apache.org/jira/browse/KAFKA-8159]

which went undiscovered for a while because there were no tests of range queries with a negative key. I actually think it's fair to say we make no guarantees about what will happen if your app makes an invalid query; however we definitely shouldn't crash on what appears to be a valid query range (ie [-1,1]), which is the key point here

Fair enough

…ey range

cadonna

LGTM

bbejeck · 2019-04-09T13:35:22Z

Java 8 failed with kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup
Java 11 passed

retest this please

bbejeck · 2019-04-09T18:49:31Z

Java 8 passed Java 11 failure unrelated

retest this please

bbejeck · 2019-04-10T14:20:30Z

Java 8 failed Execution failed for task ':core:integrationTest' Java 11 passed

retest this please

bbejeck · 2019-04-10T19:11:00Z

Merged 6521 to trunk

…pache#6521

) Due to KAFKA-8159, Streams will throw an unchecked exception when a caching layer or in-memory underlying store is queried over a range of keys from negative to positive. We should add a check for this and log it then return an empty iterator (as the RocksDB stores happen to do) rather than crash Reviewers: Bruno Cadonna <bruno@confluent.io> Bill Bejeck <bbejeck@gmail.com>

ableegoldman added 2 commits March 28, 2019 17:36

Guard against crashing on invalid key range queries in CachingXXXStores

1167df5

Added guards for range queries in in-memory stores

09ace6b

ableegoldman changed the title ~~Guard against crashing on invalid key range queries~~ [MINOR] Guard against crashing on invalid key range queries Mar 29, 2019

guozhangwang reviewed Mar 29, 2019

View reviewed changes

bbejeck reviewed Apr 4, 2019

View reviewed changes

bbejeck added the streams label Apr 4, 2019

Changed DEBUG log level to WARN

ac27e85

ableegoldman force-pushed the GuardKeyRangeQueries branch from 1e4286f to ac27e85 Compare April 4, 2019 22:33

bbejeck approved these changes Apr 5, 2019

View reviewed changes

cadonna reviewed Apr 5, 2019

View reviewed changes

ableegoldman added 2 commits April 5, 2019 12:24

Addressing comments from Bruno's review

b961d0a

Added unit tests, guard range in MemoryNavigableLRUCache, expanded er…

cef3de5

…ror messages

bbejeck reviewed Apr 5, 2019

View reviewed changes

Use LogAppender to catch and check logged messages in unit tests

f2a556b

cadonna reviewed Apr 8, 2019

View reviewed changes

ableegoldman added 4 commits April 8, 2019 16:17

Extend check & log error message to RocksDB stores as well

b2deb14

Verify empty iterator returned when fetching from negative starting k…

61e86c8

…ey range

Unit tests for key range fetch to match single-key fetch in all stores

bf38b11

Fix checkstyle

892b1e9

cadonna approved these changes Apr 9, 2019

View reviewed changes

bbejeck merged commit 9f5a69a into apache:trunk Apr 10, 2019

ableegoldman added a commit to ableegoldman/kafka that referenced this pull request Apr 10, 2019

Guard against negative key range queries and add tests following PR a…

f16f9f3

…pache#6521

vvcephei mentioned this pull request Feb 10, 2020

MINOR: revert change in log level for skipped record #8079

Closed

3 tasks

ableegoldman deleted the GuardKeyRangeQueries branch June 26, 2020 22:39

Conversation

ableegoldman commented Mar 29, 2019

Committer Checklist (excluded from commit message)

Uh oh!

ableegoldman commented Mar 29, 2019

Uh oh!

ableegoldman commented Mar 29, 2019

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

ableegoldman commented Mar 29, 2019

Uh oh!

guozhangwang commented Apr 4, 2019

Uh oh!

bbejeck 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!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Apr 5, 2019

Uh oh!

cadonna 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!

ableegoldman commented Apr 8, 2019

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!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Apr 9, 2019

Uh oh!

bbejeck commented Apr 9, 2019

Uh oh!

bbejeck commented Apr 10, 2019

Uh oh!

bbejeck commented Apr 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants