New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-12396 added a nullcheck before trying to retrieve a key #10548
Changes from 6 commits
26395a5
b06e7ae
8091bf4
c6703a0
983cc95
7142aa2
c9aef82
5e61459
3d6bc9b
12038c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,8 @@ | |
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.stream.Collectors; | ||
|
||
import static org.apache.kafka.streams.kstream.internals.WrappingNullableUtils.prepareKeySerde; | ||
import static org.apache.kafka.streams.processor.internals.metrics.StreamsMetricsImpl.maybeMeasureLatency; | ||
|
@@ -185,6 +187,7 @@ public boolean setFlushListener(final CacheFlushListener<K, V> listener, | |
|
||
@Override | ||
public V get(final K key) { | ||
Objects.requireNonNull(key, "key cannot be null"); | ||
try { | ||
return maybeMeasureLatency(() -> outerValue(wrapped().get(keyBytes(key))), time, getSensor); | ||
} catch (final ProcessorStateException e) { | ||
|
@@ -196,6 +199,7 @@ public V get(final K key) { | |
@Override | ||
public void put(final K key, | ||
final V value) { | ||
Objects.requireNonNull(key, "key cannot be null"); | ||
try { | ||
maybeMeasureLatency(() -> wrapped().put(keyBytes(key), serdes.rawValue(value)), time, putSensor); | ||
maybeRecordE2ELatency(); | ||
|
@@ -208,6 +212,7 @@ public void put(final K key, | |
@Override | ||
public V putIfAbsent(final K key, | ||
final V value) { | ||
Objects.requireNonNull(key, "key cannot be null"); | ||
final V currentValue = maybeMeasureLatency( | ||
() -> outerValue(wrapped().putIfAbsent(keyBytes(key), serdes.rawValue(value))), | ||
time, | ||
|
@@ -219,11 +224,19 @@ public V putIfAbsent(final K key, | |
|
||
@Override | ||
public void putAll(final List<KeyValue<K, V>> entries) { | ||
final List<KeyValue<K, V>> possiblyNullKeys = entries | ||
.stream() | ||
.filter(entry -> entry.key == null) | ||
.collect(Collectors.toList()); | ||
if (!possiblyNullKeys.isEmpty()) { | ||
Objects.requireNonNull(possiblyNullKeys.get(0).key, "key cannot be null"); | ||
} | ||
maybeMeasureLatency(() -> wrapped().putAll(innerEntries(entries)), time, putAllSensor); | ||
} | ||
|
||
@Override | ||
public V delete(final K key) { | ||
Objects.requireNonNull(key, "key cannot be null"); | ||
try { | ||
return maybeMeasureLatency(() -> outerValue(wrapped().delete(keyBytes(key))), time, deleteSensor); | ||
} catch (final ProcessorStateException e) { | ||
|
@@ -234,13 +247,15 @@ public V delete(final K key) { | |
|
||
@Override | ||
public <PS extends Serializer<P>, P> KeyValueIterator<K, V> prefixScan(final P prefix, final PS prefixKeySerializer) { | ||
|
||
Objects.requireNonNull(prefix, "key cannot be null"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned by @cadonna the wrapped stores, also check I think we can also remove both checks in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. KafkaStreams runtime always "wraps" any store with a corresponding Thus, when you call This architecture allows us to unify code and separate concerns. In fact, it also allows us to add/remove more "layers": we can insert a "caching layer" (cf. https://kafka.apache.org/28/documentation/streams/developer-guide/memory-mgmt.html) and a "change logging layer" (both are inserted by default). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oooooh, I see. |
||
return new MeteredKeyValueIterator(wrapped().prefixScan(prefix, prefixKeySerializer), prefixScanSensor); | ||
} | ||
|
||
@Override | ||
public KeyValueIterator<K, V> range(final K from, | ||
final K to) { | ||
Objects.requireNonNull(from, "keyFrom cannot be null"); | ||
Objects.requireNonNull(to, "keyTo cannot be null"); | ||
return new MeteredKeyValueIterator( | ||
wrapped().range(Bytes.wrap(serdes.rawKey(from)), Bytes.wrap(serdes.rawKey(to))), | ||
rangeSensor | ||
|
@@ -250,6 +265,8 @@ public KeyValueIterator<K, V> range(final K from, | |
@Override | ||
public KeyValueIterator<K, V> reverseRange(final K from, | ||
final K to) { | ||
Objects.requireNonNull(from, "keyFrom cannot be null"); | ||
Objects.requireNonNull(to, "keyTo cannot be null"); | ||
return new MeteredKeyValueIterator( | ||
wrapped().reverseRange(Bytes.wrap(serdes.rawKey(from)), Bytes.wrap(serdes.rawKey(to))), | ||
rangeSensor | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -472,11 +472,26 @@ public void shouldThrowNullPointerOnRemoveIfKeyIsNull() { | |
assertThrows(NullPointerException.class, () -> store.remove(null)); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnPutIfWrappedKeyIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.put(new Windowed<>(null, new SessionWindow(0, 0)), "a")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test remind me, that the |
||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnRemoveIfWrappedKeyIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.remove(new Windowed<>(null, new SessionWindow(0, 0)))); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnFetchIfKeyIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.fetch(null)); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnFetchSessionIfKeyIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.fetchSession(null, 0, Long.MAX_VALUE)); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnFetchRangeIfFromIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.fetch(null, "to")); | ||
|
@@ -487,6 +502,21 @@ public void shouldThrowNullPointerOnFetchRangeIfToIsNull() { | |
assertThrows(NullPointerException.class, () -> store.fetch("from", null)); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnBackwardFetchIfKeyIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.backwardFetch(null)); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnBackwardFetchIfFromIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.backwardFetch(null, "to")); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnBackwardFetchIfToIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.backwardFetch("from", null)); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnFindSessionsIfKeyIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.findSessions(null, 0, 0)); | ||
|
@@ -502,6 +532,21 @@ public void shouldThrowNullPointerOnFindSessionsRangeIfToIsNull() { | |
assertThrows(NullPointerException.class, () -> store.findSessions("a", null, 0, 0)); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnBackwardFindSessionsIfKeyIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.backwardFindSessions(null, 0, 0)); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnBackwardFindSessionsRangeIfFromIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.backwardFindSessions(null, "a", 0, 0)); | ||
} | ||
|
||
@Test | ||
public void shouldThrowNullPointerOnBackwardFindSessionsRangeIfToIsNull() { | ||
assertThrows(NullPointerException.class, () -> store.backwardFindSessions("a", null, 0, 0)); | ||
} | ||
|
||
private interface CachedSessionStore extends SessionStore<Bytes, byte[]>, CachedStateStore<byte[], byte[]> { } | ||
|
||
@SuppressWarnings("unchecked") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could simplify this to a one liner?