I think the KIP needs the rejected alternatives section to have more
detail. For example, another option would be something like the following,
which works great as long as one overrides one of the methods, but pretty
bad if one doesn't. :)
default T deserialize(String topic, byte[] data) {
return deserialize(topic, null, data);
}
default T deserialize(String topic, Headers headers, byte[] data) { // This
is the new method
return deserialize(topic, data);
}
On Thu, Aug 23, 2018 at 3:57 AM Viktor Somogyi-Vass <viktorsomo...@gmail.com>
wrote:
> 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
> > > > >
> > > >
> > >
> >
>
