MINOR: Improve ValueTimestampHeader Serdes#21515
Conversation
|
|
||
| @Override | ||
| public ValueTimestampHeaders<V> deserialize(final String topic, final Headers headers, final byte[] valueTimestampHeaders) { | ||
| throw new UnsupportedOperationException("Headers are encoded inside `valueTimestampHeaders` byte[] array and cannot be passed as parameter. Use `deserialize(String topic, byte[] valueTimestampHeaders)` instead."); |
There was a problem hiding this comment.
Frankly, I am not 100% if this is a too restrictive check?
We could also check if header == null || headers.toArray().size == 0 and allow the call for this case? Same for the serializer. Thoughts?
There was a problem hiding this comment.
What is the worries here? 1) We call this method while the passed headers is different with the headers encoded in valueTimestampHeaders? or 2) the general ambiguty of having two headers?
if we are more concerned with 2nd issue, we must be explicit about API misuse and throw exception anyway, but here we generally are breaking Liskov Substitution Principle.
|
The concern is both of the points you mentioned. -- And yes, I agree that it's not ideal... But it might be "ok-ish", given that these classes an KS internal only? |
Ok, so let’s go ahead and be strict and throw an exception anyway, since we know this method should never be called. Thanks for being so thorough. |
frankvicky
left a comment
frankvicky
left a comment
There was a problem hiding this comment.
This changes make sense but I wondering that we already have a default impl in the interface which will invoke deserialize(String, Object). Do we need this override?
default T deserialize(String topic, Headers headers, byte[] data) {
return deserialize(topic, data);
}
|
@frankvicky -- You are right; technically we don't need the overload given the default impl. The idea was to make it explicit on the class itself, that both methods are not supported (also guards us for the case, that the default impl would get changed; even if I don't expect this to happen as would raise backward compatibility questions). |
This PR disallows to pass in Headers objects into serialize() and deserializer() to ensure we don't accidentally introduce bugs in our own code.
mjsax
force-pushed
the
minor-improve-valuetimestampheader-serdes
branch
from
7ae9de0 to
aa7e381
Compare
| final byte[] bytes = readBytes(buffer, buffer.remaining()); | ||
|
|
||
| return deserializer.deserialize("", bytes); | ||
| return deserializer.deserialize("", headers, bytes); |
There was a problem hiding this comment.
One more fix... @UladzislauBlok pointed me to it! Great find.
Base on this, I am wondering about "lazy header deserialization" idea... Left a comment on https://issues.apache.org/jira/browse/KAFKA-20155
3 participants
This PR disallows to pass in Headers objects into serialize() and
deserializer() to ensure we don't accidentally introduce bugs in our own
code.
It also fixes a small bug, and now correctly passed Headers into the
value deserizlier.
Co-authored-by: Uladzislau Blok blokv75@gmail.com Reviewers: Alieh
Saeedi asaeedi@confluent.io, TengYao Chi frankvicky@apache.org