[KAFKA-6923] Refactoring Serializer/Deserializer - #5494
Conversation
026a1d7 to
a7b0d39
Compare
satishd
left a comment
satishd
left a comment
There was a problem hiding this comment.
Thanks @viktorsomogyi for the PR.
Overall LGTM, good to have tests for the introduced methods with Serializer and Deserializer.
|
@satishd thanks very much for the review. Do you have any specific test case in mind I should implement? |
|
@viktorsomogyi As you have added new methods in Serializer/Deserializer, it is good to add a test to make sure those methods are invoked from producer and consumer respectively in an expected manner. You may want to look at the tests added earlier for ExtendedSerializer/ExtendedDeserializer here(1). |
|
@satishd thanks, I'll update my PR soon with added tests. |
aa82252 to
6b632a6
Compare
|
@satishd rebased on latest trunk + added some tests. Will add more based on the outcome of the KIP discussions. |
|
retest this please |
| * @return deserialized typed data; may be null | ||
| */ | ||
| T deserialize(String topic, byte[] data); | ||
| default T deserialize(String topic, byte[] data) { |
There was a problem hiding this comment.
hey @viktorsomogyi ,
Sorry I missed the KIP discussion, but I just read the KIP over and didn't see a explanation of this:
Can you explain the reason to provide a default implementation of this method?
Thanks,
-John
There was a problem hiding this comment.
Ah, I just now saw the discussion in the vote thread. It sounds like you're going to remove this default. Sorry about that.
There was a problem hiding this comment.
Yes. Thank you for the review though. :)
I'll updte this piece of code soon.
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
Thanks @viktorsomogyi. I had only one comment.
| } | ||
|
|
||
| @Override | ||
| public byte[] serialize(String topic, Headers headers, ByteBuffer data) { |
There was a problem hiding this comment.
This may have been left behind from a previous iteration, but shouldn't we just rely on the default implementation?
There was a problem hiding this comment.
Indeed, I'll correct it.
|
retest this please |
satishd
left a comment
satishd
left a comment
There was a problem hiding this comment.
Thanks @viktorsomogyi for addressing review comments.
+1 LGTM
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the patch and the KIP.
|
retest this please |
|
@viktorsomogyi I think it would be good to include an upgrade note. |
This patch implements KIP-336. It adds a default implementation to the Serializer/Deserializer interface to support the use of headers and it deprecates the ExtendedSerializer and ExtendedDeserializer interfaces for later removal. Reviewers: Satish Duggana <sduggana@hortonworks.com>, John Roesler <john@confluent.io>, Jason Gustafson <jason@confluent.io>
5 participants
This is the implementation of KIP-336.
Committer Checklist (excluded from commit message)