UladzislauBlok commented on code in PR #21971:
URL: https://github.com/apache/kafka/pull/21971#discussion_r3597595167
##########
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:
> everywhere when prefixKeySerializer.serialize(...)
I did it everywhere, where we call it beside `RocksDBStore`. From what I
understand while implementing this `RocksDBStore#prefixScan` is not really
called anywhere directly, as all `wrappers` (like `metered`, `caching`, etc...
call `range`... correct me if I'm wrong here)
> put the serialization into the metered
If you're asking from perspective of metered vs underlying: this is because
underlying doesn't have an access to context, meaning we can't really get
headers
If you're asking from perspective why
`MeteredTimestampedKeyValueStoreWithHeaders` and not `MeteredKeyValueStore`, so
that because:
- That's what jira ticket says (sorry for dumb argument :) )
- We know that headers are critical for headers-full stores, so we
deferentially need to fix `MeteredTimestampedKeyValueStoreWithHeaders`, while
headers propagation in `MeteredKeyValueStore` is nice to have
- Actually we can't just move code to higher level (fix in
`MeteredKeyValueStore` and let child classes inherit the fix), as `prefixScan`
is parallel implemented in both basic and headers versions
See:
https://github.com/apache/kafka/blob/bd5fcec64e0a2ec057ca9f88da345926d8c58342/streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredTimestampedKeyValueStoreWithHeaders.java#L584
See:
https://github.com/apache/kafka/blob/bd5fcec64e0a2ec057ca9f88da345926d8c58342/streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredKeyValueStore.java#L402
I can fix basic store as well, but we still need to keep the fix on
`MeteredTimestampedKeyValueStoreWithHeaders`
WDYT?
--
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]
