KAFKA-20173: Ensure Metered kv-stores pass headers correctly#21768
KAFKA-20173: Ensure Metered kv-stores pass headers correctly#21768mjsax wants to merge 1 commit intoapache:trunkfrom
Conversation
Ensures that all Metered KV-stores (plain, ts, headers, version) pass headers into de/serializers.
| protected Sensor getSensor; | ||
| protected Sensor deleteSensor; | ||
| private Sensor putAllSensor; | ||
| protected Sensor putAllSensor; |
There was a problem hiding this comment.
Need access in MeteredKeyValueTimestampStoreWithHeaders
| if (rawResult.isSuccess()) { | ||
| final KeyValueIterator<Bytes, byte[]> iterator = rawResult.getResult(); | ||
| final KeyValueIterator<K, V> resultIterator = new MeteredKeyValueTimestampedIterator( | ||
| final KeyValueIterator<K, V> resultIterator = new MeteredKeyValueStoreIterator( |
There was a problem hiding this comment.
Side cleanup: I think there is no reason to have MeteredKeyValueTimestampedIterator (removing blow), and we can just use existing MeteredKeyValueStoreIterator. -- We don't have the case to bridge between different value types in MeteredKeyValueStore.
| Objects.requireNonNull(prefix, "prefix cannot be null"); | ||
| Objects.requireNonNull(prefixKeySerializer, "prefixKeySerializer cannot be null"); | ||
| return new MeteredKeyValueIterator(wrapped().prefixScan(prefix, prefixKeySerializer), prefixScanSensor); | ||
| return new MeteredKeyValueStoreIterator(wrapped().prefixScan(prefix, prefixKeySerializer), prefixScanSensor); |
There was a problem hiding this comment.
Just renaming, to align to established naming patterns across the different kv-metered stores.
| public ValueTimestampHeaders<V> get(final K key) { | ||
| Objects.requireNonNull(key, "key cannot be null"); | ||
| try { | ||
| return maybeMeasureLatency(() -> deserializeValue(wrapped().get(serializeKey(key, new RecordHeaders()))), time, getSensor); |
There was a problem hiding this comment.
We need to overwrite get() to be able to pass in headers into serializeKey(...) -- For get() it might not be strictly necessary, but if forces us to make a conscious decision what to pass -- it's a side-effect of disallowing to call serializeKey w/o headers.
There was a problem hiding this comment.
Wondering if we would want to pass context.headers() instead of new RecordHeaders(), similar to what we did in #21684
If yes, applies elsewhere in this class, too.
| try { | ||
| final Headers headers = value != null ? value.headers() : new RecordHeaders(); | ||
| maybeMeasureLatency(() -> wrapped().put(keyBytes(key, headers), serdes.rawValue(value, headers)), time, putSensor); | ||
| maybeMeasureLatency(() -> wrapped().put(serializeKey(key, headers), serdes.rawValue(value, headers)), time, putSensor); |
There was a problem hiding this comment.
Unifying the existing keyBytes method with the naming of MeteredKeyValueStore which uses serializeKey
| if (rawResult.isSuccess()) { | ||
| final KeyValueIterator<Bytes, byte[]> iterator = rawResult.getResult(); | ||
| final KeyValueIterator<K, V> resultIterator = new MeteredTimestampedKeyValueStoreWithHeadersIterator( | ||
| final KeyValueIterator<K, V> resultIterator = new MeteredTimestampedKeyValueStoreWithHeadersQueryIterator( |
There was a problem hiding this comment.
Just renaming to aling naming patterns.
| Objects.requireNonNull(prefix, "prefix cannot be null"); | ||
| Objects.requireNonNull(prefixKeySerializer, "prefixKeySerializer cannot be null"); | ||
| return new MeteredValueTimestampHeadersIterator( | ||
| return new MeteredTimestampedKeyValueStoreWithHeadersIterator( |
There was a problem hiding this comment.
Just renaming to align naming patterns
|
|
||
| final KeyValue<Bytes, byte[]> keyValue = iter.next(); | ||
| final ValueTimestampHeaders<V> valueTimestampHeaders = valueTimestampHeadersDeserializer.apply(keyValue.value); | ||
| final Headers headers = valueTimestampHeaders != null ? valueTimestampHeaders.headers() : new RecordHeaders(); |
There was a problem hiding this comment.
This can never be null. We know that iter.next cannot return null (this would be a "non existing row" -- null-values are deletes).
Thus simplifying the code (we don't have any null checks for this case in older code for plain- and ts-store either.
Similar below
| protected Bytes keyBytes(final K key, final Headers headers) { | ||
| @Override | ||
| protected Bytes serializeKey(final K key) { | ||
| throw new UnsupportedOperationException("MeteredTimestampedKeyValueStoreWithHeaders required to pass in Headers when serializing a key."); |
There was a problem hiding this comment.
This was actually surfacing the missing overrides, highlighting the bug to not override delete. So a good guard to have in place (little bit annoying side effect that it forces us to override put/putAll, too, but I think it's worth the prices to pay)
|
|
||
| @Override | ||
| protected K deserializeKey(final byte[] rawKey) { | ||
| throw new UnsupportedOperationException("MeteredTimestampedKeyValueStoreWithHeaders required to pass in Headers when deserializing a key."); |
There was a problem hiding this comment.
Similar guard -- this did not surface any issues.
1 participant
Ensures that all Metered KV-stores (plain, ts, headers, version) pass
headers into de/serializers.