mjsax commented on code in PR #21684:
URL: https://github.com/apache/kafka/pull/21684#discussion_r2903043704
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredKeyValueStore.java:
##########
@@ -420,18 +432,26 @@ public void close() {
}
}
- protected V outerValue(final byte[] value) {
- return value != null ? serdes.valueFrom(value, new RecordHeaders()) :
null;
+ protected byte[] serializeValue(final V value) {
+ return value != null ? serdes.rawValue(value,
internalContext.headers()) : null;
Review Comment:
This is the key question -- should we pass `context.headers()` or `new
RecordHeaders` for non-header stores. Both solutions have advantages and
disadvantages.
Using `new RecordHeaders` is strictly more backward compatible; the idea to
pass in `context.headers()` feels like a "bug fix" though -- we should have
always done this IMHO. That's why I opted for this solution.
Of course, we can also "fix" this "bug" by arguing: we stay 100% backward
compatible and pass in `new RecordHeaders` and user get the fix by enabling the
new headers stores...
Let me know what you think.
--
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]
