mjsax commented on code in PR #21971:
URL: https://github.com/apache/kafka/pull/21971#discussion_r3592189930
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/CachingKeyValueStore.java:
##########
@@ -438,6 +438,7 @@ private <PS extends Serializer<P>, P>
KeyValueIterator<Bytes, byte[]> prefixScan
final ReadOnlyKeyValueStore<Bytes, byte[]> underlying) {
validateStoreOpen();
final KeyValueIterator<Bytes, byte[]> storeIterator =
underlying.prefixScan(prefix, prefixKeySerializer);
+ // headers aren't needed because the prefix already arrives serialized
Review Comment:
But this is only correct _if_ the most outer store is
`MeteredTimestampedKeyValueStoreWithHeaders` for which this PR changes the
`prefixScan` impl. For the other metered-kv-stores this is not correct.
And I am wondering why we want to put the serialization into the metered
layer now? Guess not wrong, because that's is what the metered layer does in
general, but for this case, we should also update `MeteredKeyValueStore.java`
accordingly? (`MeteredTimestampedKeyValueStore` does not overwrite
`prefixScan()`)?
If we add this comment, it seems we would need to add it elsewhere, too, eg
`RocksDBStore` and maybe also others -- ie, everywhere when
`prefixKeySerializer.serialize(...)` is used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
