Also, we may consider deprecating the deserialize method that does not take
headers. Yes, it's a convenience, but it also adds confusion.
Ismael
On Thu, Aug 23, 2018 at 6:48 AM Ismael Juma <ism...@juma.me.uk> wrote:
> 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
>> > > > >
>> > > >
>> > >
>> >
>>
>
