Hi Jason, Thanks for the feedback. 1. I chose to return null here because according to the documentation it may return null data, therefore the users of this methods are perpared for getting a null. Thinking of it though it may be better to throw an exception by default because it'd indicate a programming error. However, would that be a backward incompatible change? I'm simply thinking of this because this is a new behavior that we'd introduce but I'm not sure yet if it'd cause problems. Do you think it'd make sense to do the same in `serialize`? 2. Yes, I believe that is covered in KIP-331: https://cwiki.apache.org/confluence/display/KAFKA/KIP-331+Add+default+implementation+to+close%28%29+and+configure%28%29+for+Serializer%2C+Deserializer+and+Serde
Cheers, Viktor On Wed, Aug 22, 2018 at 6:11 PM Jason Gustafson <ja...@confluent.io> wrote: > Hey Viktor, > > This is a nice cleanup. Just a couple quick questions: > > 1. Rather than returning null for the default `deserialize(topic, data)`, > would it be better to throw UnsupportedOperationException? I assume that > internally we'll always invoke the api which takes headers. Similarly for > `serialize(topic, data)`. > 2. Would it make sense to have default no-op implementations for > `configure` and `close`? > > Thanks, > Jason > > On Wed, Aug 22, 2018 at 5:27 AM, Satish Duggana <satish.dugg...@gmail.com> > wrote: > > > +1 > > > > On Wed, Aug 22, 2018 at 4:45 PM, Ted Yu <yuzhih...@gmail.com> wrote: > > > > > +1 > > > -------- Original message --------From: Kamal Chandraprakash < > > > kamal.chandraprak...@gmail.com> Date: 8/22/18 3:19 AM (GMT-08:00) > To: > > > dev@kafka.apache.org Subject: Re: [VOTE] KIP-336: Consolidate > > > ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer > > > +1 > > > > > > Thanks for the KIP! > > > > > > On Wed, Aug 22, 2018 at 2:48 PM Viktor Somogyi-Vass < > > > viktorsomo...@gmail.com> > > > wrote: > > > > > > > Hi All, > > > > > > > > I'd like to start a vote on this KIP ( > > > > https://cwiki.apache.org/confluence/pages/viewpage. > > > action?pageId=87298242) > > > > which aims to refactor ExtendedSerializer/Serializer and > > > > ExtendedDeserializer/Deserializer. > > > > > > > > To summarize what's the motivation: > > > > > > > > When headers were introduced by KIP-82 the ExtendedSerializer and > > > > ExtendedDeserializer classes were created in order to keep interface > > > > compatibility but still add `T deserialize(String topic, Headers > > headers, > > > > byte[] data);` and `byte[] serialize(String topic, Headers headers, T > > > > data);` methods that consume the headers for > > > serialization/deserialization. > > > > The reason for doing so was that Kafka at that time needed be > > compatbile > > > > with Java 7. Since we're not compiling on Java 7 anymore (KAFKA-4423) > > > we'll > > > > try consolidate the way we're using these in a backward compatible > > > fashion: > > > > deprecating the Extended* classes and moving the aforementioned > methods > > > up > > > > in the class hierarchy. > > > > > > > > I'd be happy to get votes or additional feedback on this. > > > > > > > > Viktor > > > > > > > > > >
