Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Viktor Somogyi-Vass]

gt; > >
> > >> > > > > > > > > > > > Hey Viktor,
> > >> > > > > > > > > > > >
> > >> > > > > > > > > > > > Thinking about it a little more, I wonder if we
> > >> should
> > >> > > just
> > >> > > > > not
> > >> > > > > > > > > > provide a
> > >> > > > > > > > > > > > default method for serialize(topic, data) and
> > >> > > > > > deserialize(topic,
> > >> > > > > > > > > data).
> > >> > > > > > > > > > > > Implementing these methods is a trivial burden
> for
> > >> users
> > >> > > > and
> > >> > > > > it
> > >> > > > > > > > feels
> > >> > > > > > > > > > > like
> > >> > > > > > > > > > > > there's no good solution which allows both
> methods
> > >> to
> > >> > > have
> > >> > > > > > > default
> > >> > > > > > > > > > > > implementations.
> > >> > > > > > > > > > > >
> > >> > > > > > > > > > > > Also, ack on KIP-331. Thanks for the pointer.
> > >> > > > > > > > > > > >
> > >> > > > > > > > > > > > -Jason
> > >> > > > > > > > > > > >
> > >> > > > > > > > > > > > On Thu, Aug 23, 2018 at 12:30 PM, Viktor
> > >> Somogyi-Vass <
> > >> > > > > > > > > > > > viktorsomo...@gmail.com> wrote:
> > >> > > > > > > > > > > >
> > >> > > > > > > > > > > >> Hi Ismael,
> > >> > > > > > > > > > > >>
> > >> > > > > > > > > > > >> Regarding the deprecation of the 2 parameter
> > >> method:
> > >> > > > should
> > >> > > > > we
> > >> > > > > > > do
> > >> > > > > > > > > this
> > >> > > > > > > > > > > >> with
> > >> > > > > > > > > > > >> the Serializer interface as well?
> > >> > > > > > > > > > > >>
> > >> > > > > > > > > > > >> I've updated the "Rejected Alternatives" with a
> > >> few.
> > >> > > > > > > > > > > >> I've added this circular reference one too but
> > >> actually
> > >> > > > > > there's
> > >> > > > > > > a
> > >> > > > > > > > > way
> > >> > > > > > > > > > > >> (pretty heavyweight) by adding a guard class
> that
> > >> > > prevents
> > >> > > > > > > > recursive
> > >> > > > > > > > > > > >> invocation of either methods. I've tried this
> out
> > >> but it
> > >> > > > > seems
> > >> > > > > > > to
> > >> > > > > > > > me
> > >> > > > > > > > > > an
> > >> > > > > > > > > > > >> overshoot. So just for the sake of completeness
> > >> I'll
> > >> > > copy
> > >> > > > it
> > >> > > > > > > here.
> > >> > > > > > > > > :)
> > >> > > > > > > > > > > >>
> > >> > > > > > > > > > > >> public interface Deserializer extends
> > Closeable
> > >> {
> > >> > > > > > > > > > > >>
> > >> > > > > > > > > > > >> class Guard {
> > >> > > > > > > > > > > >>
> > >> > > > > > > > > > > >> private Se

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Ismael Juma]

gt; > > > > > > > >> Regarding the deprecation of the 2 parameter
> >> method:
> >> > > > should
> >> > > > > we
> >> > > > > > > do
> >> > > > > > > > > this
> >> > > > > > > > > > > >> with
> >> > > > > > > > > > > >> the Serializer interface as well?
> >> > > > > > > > > > > >>
> >> > > > > > > > > > > >> I've updated the "Rejected Alternatives" with a
> >> few.
> >> > > > > > > > > > > >> I've added this circular reference one too but
> >> actually
> >> > > > > > there's
> >> > > > > > > a
> >> > > > > > > > > way
> >> > > > > > > > > > > >> (pretty heavyweight) by adding a guard class that
> >> > > prevents
> >> > > > > > > > recursive
> >> > > > > > > > > > > >> invocation of either methods. I've tried this out
> >> but it
> >> > > > > seems
> >> > > > > > > to
> >> > > > > > > > me
> >> > > > > > > > > > an
> >> > > > > > > > > > > >> overshoot. So just for the sake of completeness
> >> I'll
> >> > > copy
> >> > > > it
> >> > > > > > > here.
> >> > > > > > > > > :)
> >> > > > > > > > > > > >>
> >> > > > > > > > > > > >> public interface Deserializer extends
> Closeable
> >> {
> >> > > > > > > > > > > >>
> >> > > > > > > > > > > >> class Guard {
> >> > > > > > > > > > > >>
> >> > > > > > > > > > > >> private Set objects =
> >> > > > > > > > > Collections.synchronizedSet(new
> >> > > > > > > > > > > >> HashSet<>()); // might as well use concurrent
> >> hashmap
> >> > > > > > > > > > > >>
> >> > > > > > > > > > > >> private void methodCallInProgress(Object
> >> x) {
> >> > > > > > > > > > > >> objects.add(x);
> >> > > > > > > > > > > >> }
> >> > > > > > > > > > > >>
> >> > > > > > > > > > > >> private boolean
> >> isMethodCallInProgress(Object
> >> > > x) {
> >> > > > > > > > > > > >> return objects.contains(x);
> >> > > > > > > > > > > >> }
> >> > > > > > > > > > > >>
> >> > > > > > > > > > > >> private void
> >> clearMethodCallInProgress(Object
> >> > > x) {
> >> > > > > > > > > > > >> objects.remove(x);
> >> > > > > > > > > > > >> }
> >> > > > > > > > > > > >>
> >> > > > > > > > > > > >> private  T guard(Supplier
> supplier) {
> >> > > > > > > > > > > >> if
> >> (GUARD.isMethodCallInProgress(this)) {
> >> > > > > > > > > > > >> throw new
> >> IllegalStateException("You
> >> > > must
> >> > > > > > > > implement
> >> > > > > > > > > > one
> >> > > > > > > > > > > of
> >> > > > > > > > > > > >> the deserialize methods");
> >> > > > > > > > > > > >> } else {
> >> > > > > > > > > > > >> try {
> >> > > > > > > > > > > >>
> >>  GUARD.method

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Viktor Somogyi-Vass]

t; > > > > > > > > >>
>> > > > > > > > > > > >> class Guard {
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> private Set objects =
>> > > > > > > > > Collections.synchronizedSet(new
>> > > > > > > > > > > >> HashSet<>()); // might as well use concurrent
>> hashmap
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> private void methodCallInProgress(Object
>> x) {
>> > > > > > > > > > > >> objects.add(x);
>> > > > > > > > > > > >> }
>> > > > > > > > > > > >>
>> > > > > > > > > > > >>     private boolean
>> isMethodCallInProgress(Object
>> > > x) {
>> > > > > > > > > > > >> return objects.contains(x);
>> > > > > > > > > > > >> }
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> private void
>> clearMethodCallInProgress(Object
>> > > x) {
>> > > > > > > > > > > >> objects.remove(x);
>> > > > > > > > > > > >> }
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> private  T guard(Supplier supplier) {
>> > > > > > > > > > > >> if
>> (GUARD.isMethodCallInProgress(this)) {
>> > > > > > > > > > > >> throw new
>> IllegalStateException("You
>> > > must
>> > > > > > > > implement
>> > > > > > > > > > one
>> > > > > > > > > > > of
>> > > > > > > > > > > >> the deserialize methods");
>> > > > > > > > > > > >> } else {
>> > > > > > > > > > > >> try {
>> > > > > > > > > > > >>
>>  GUARD.methodCallInProgress(this);
>> > > > > > > > > > > >> return supplier.get();
>> > > > > > > > > > > >> } finally {
>> > > > > > > > > > > >>
>> > >  GUARD.clearMethodCallInProgress(this);
>> > > > > > > > > > > >> }
>> > > > > > > > > > > >> }
>> > > > > > > > > > > >> }
>> > > > > > > > > > > >> }
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> Guard GUARD = new Guard();
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> void configure(Map configs, boolean
>> > > isKey);
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> default T deserialize(String topic, byte[]
>> data) {
>> > > > > > > > > > > >> return GUARD.guard(() -> deserialize(topic,
>> > > null,
>> > > > > > > data));
>> > > > > > > > > > > >> }
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> default T deserialize(String topic, Headers
>> headers,
>> > > > > > byte[]
>> > > > > > > > > data)
>> > > > > > > > > > {
>> > > > > > > > > > > >> return GUARD.guard(() -> deserialize(topic,
>> > > > data));
>> > > > > > > > > > > >> }
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> @Override
>> > > > > > > > > > > >> void close();
>> > > > > > > > > > > >> }
>> > > > > > > > > > > >>
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> Cheers,
>> > > > > > > > > > > >> Viktor
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> On Thu, Aug 23, 2018 at 3:50 PM Ismael Juma <
>> > > > > > ism...@juma.me.uk>
>> > > > > > > > > > wrote:
>> > > > > > > > > > > >>
>> > > > > > > > > > > >> > 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
>> > > > > > > > > > > >> > >> > > > >
>> > > > > > > > > > > >> > >> > > >
>> > > > > > > > > > > >> > >> > >
>> > > > > > > > > > > >> > >> >
>> > > > > > > > > > > >> > >>
>> > > > > > > > > > > >> > >
>> > > > > > > > > > > >> >
>> > > > > > > > > > > >>
>> > > > > > > > > > > >
>> > > > > > > > > > > >
>> > > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>>
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Viktor Somogyi-Vass]

;> private  T guard(Supplier supplier) {
> > > > > > > > > > > >> if (GUARD.isMethodCallInProgress(this))
> {
> > > > > > > > > > > >> throw new IllegalStateException("You
> > > must
> > > > > > > > implement
> > > > > > > > > > one
> > > > > > > > > > > of
> > > > > > > > > > > >> the deserialize methods");
> > > > > > > > > > > >> } else {
> > > > > > > > > > > >> try {
> > > > > > > > > > > >>
>  GUARD.methodCallInProgress(this);
> > > > > > > > > > > >> return supplier.get();
> > > > > > > > > > > >> } finally {
> > > > > > > > > > > >>
> > >  GUARD.clearMethodCallInProgress(this);
> > > > > > > > > > > >> }
> > > > > > > > > > > >> }
> > > > > > > > > > > >> }
> > > > > > > > > > > >> }
> > > > > > > > > > > >>
> > > > > > > > > > > >> Guard GUARD = new Guard();
> > > > > > > > > > > >>
> > > > > > > > > > > >> void configure(Map configs, boolean
> > > isKey);
> > > > > > > > > > > >>
> > > > > > > > > > > >> default T deserialize(String topic, byte[]
> data) {
> > > > > > > > > > > >> return GUARD.guard(() -> deserialize(topic,
> > > null,
> > > > > > > data));
> > > > > > > > > > > >> }
> > > > > > > > > > > >>
> > > > > > > > > > > >> default T deserialize(String topic, Headers
> headers,
> > > > > > byte[]
> > > > > > > > > data)
> > > > > > > > > > {
> > > > > > > > > > > >> return GUARD.guard(() -> deserialize(topic,
> > > > data));
> > > > > > > > > > > >> }
> > > > > > > > > > > >>
> > > > > > > > > > > >> @Override
> > > > > > > > > > > >> void close();
> > > > > > > > > > > >> }
> > > > > > > > > > > >>
> > > > > > > > > > > >>
> > > > > > > > > > > >> Cheers,
> > > > > > > > > > > >> Viktor
> > > > > > > > > > > >>
> > > > > > > > > > > >> On Thu, Aug 23, 2018 at 3:50 PM Ismael Juma <
> > > > > > ism...@juma.me.uk>
> > > > > > > > > > wrote:
> > > > > > > > > > > >>
> > > > > > > > > > > >> > 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
> > > > > > > > > > > >> > >> > > > >
> > > > > > > > > > > >> > >> > > >
> > > > > > > > > > > >> > >> > >
> > > > > > > > > > > >> > >> >
> > > > > > > > > > > >> > >>
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> >
> > > > > > > > > > > >>
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Harsha]

> > > >>
> > > > > > > > > > >> I've updated the "Rejected Alternatives" with a few.
> > > > > > > > > > >> I've added this circular reference one too but actually
> > > > > there's
> > > > > > a
> > > > > > > > way
> > > > > > > > > > >> (pretty heavyweight) by adding a guard class that
> > prevents
> > > > > > > recursive
> > > > > > > > > > >> invocation of either methods. I've tried this out but it
> > > > seems
> > > > > > to
> > > > > > > me
> > > > > > > > > an
> > > > > > > > > > >> overshoot. So just for the sake of completeness I'll
> > copy
> > > it
> > > > > > here.
> > > > > > > > :)
> > > > > > > > > > >>
> > > > > > > > > > >> public interface Deserializer extends Closeable {
> > > > > > > > > > >>
> > > > > > > > > > >> class Guard {
> > > > > > > > > > >>
> > > > > > > > > > >> private Set objects =
> > > > > > > > Collections.synchronizedSet(new
> > > > > > > > > > >> HashSet<>()); // might as well use concurrent hashmap
> > > > > > > > > > >>
> > > > > > > > > > >> private void methodCallInProgress(Object x) {
> > > > > > > > > > >> objects.add(x);
> > > > > > > > > > >> }
> > > > > > > > > > >>
> > > > > > > > > > >> private boolean isMethodCallInProgress(Object
> > x) {
> > > > > > > > > > >> return objects.contains(x);
> > > > > > > > > > >> }
> > > > > > > > > > >>
> > > > > > > > > > >> private void clearMethodCallInProgress(Object
> > x) {
> > > > > > > > > > >> objects.remove(x);
> > > > > > > > > > >> }
> > > > > > > > > > >>
> > > > > > > > > > >> private  T guard(Supplier supplier) {
> > > > > > > > > > >> if (GUARD.isMethodCallInProgress(this)) {
> > > > > > > > > > >> throw new IllegalStateException("You
> > must
> > > > > > > implement
> > > > > > > > > one
> > > > > > > > > > of
> > > > > > > > > > >> the deserialize methods");
> > > > > > > > > > >> } else {
> > > > > > > > > > >> try {
> > > > > > > > > > >> GUARD.methodCallInProgress(this);
> > > > > > > > > > >> return supplier.get();
> > > > > > > > > > >>     } finally {
> > > > > > > > > > >>
> >  GUARD.clearMethodCallInProgress(this);
> > > > > > > > > > >> }
> > > > > > > > > > >> }
> > > > > > > > > > >> }
> > > > > > > > > > >> }
> > > > > > > > > > >>
> > > > > > > > > > >> Guard GUARD = new Guard();
> > > > > > > > > > >>
> > > > > > > > > > >> void configure(Map configs, boolean
> > isKey);
> > > > > > > > > > >>
> > > > > > > > > > >> default T deserialize(String topic, byte[] data) {
> > > > > > > > > > >> return GUARD.guard(() -> deserialize(topic,
> > null,
> > > > > > data));
> > > > > > > > > > >> }
> > > > > > > > > > >>
&g

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Attila Sasvári]

<>()); // might as well use concurrent hashmap
> > > > > > > > > >>
> > > > > > > > > >> private void methodCallInProgress(Object x) {
> > > > > > > > > >> objects.add(x);
> > > > > > > > > >> }
> > > > > > > > > >>
> > > > > > > > > >> private boolean isMethodCallInProgress(Object
> x) {
> > > > > > > > > >> return objects.contains(x);
> > > > > > > > > >> }
> > > > > > > > > >>
> > > > > > > > > >> private void clearMethodCallInProgress(Object
> x) {
> > > > > > > > > >> objects.remove(x);
> > > > > > > > > >> }
> > > > > > > > > >>
> > > > > > > > > >> private  T guard(Supplier supplier) {
> > > > > > > > > >> if (GUARD.isMethodCallInProgress(this)) {
> > > > > > > > > >> throw new IllegalStateException("You
> must
> > > > > > implement
> > > > > > > > one
> > > > > > > > > of
> > > > > > > > > >> the deserialize methods");
> > > > > > > > > >> } else {
> > > > > > > > > >> try {
> > > > > > > > > >> GUARD.methodCallInProgress(this);
> > > > > > > > > >> return supplier.get();
> > > > > > > > > >> } finally {
> > > > > > > > > >>
>  GUARD.clearMethodCallInProgress(this);
> > > > > > > > > >> }
> > > > > > > > > >> }
> > > > > > > > > >> }
> > > > > > > > > >> }
> > > > > > > > > >>
> > > > > > > > > >> Guard GUARD = new Guard();
> > > > > > > > > >>
> > > > > > > > > >> void configure(Map configs, boolean
> isKey);
> > > > > > > > > >>
> > > > > > > > > >> default T deserialize(String topic, byte[] data) {
> > > > > > > > > >> return GUARD.guard(() -> deserialize(topic,
> null,
> > > > > data));
> > > > > > > > > >> }
> > > > > > > > > >>
> > > > > > > > > >> default T deserialize(String topic, Headers headers,
> > > > byte[]
> > > > > > > data)
> > > > > > > > {
> > > > > > > > > >> return GUARD.guard(() -> deserialize(topic,
> > data));
> > > > > > > > > >> }
> > > > > > > > > >>
> > > > > > > > > >> @Override
> > > > > > > > > >> void close();
> > > > > > > > > >> }
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >> Cheers,
> > > > > > > > > >> Viktor
> > > > > > > > > >>
> > > > > > > > > >> On Thu, Aug 23, 2018 at 3:50 PM Ismael Juma <
> > > > ism...@juma.me.uk>
> > > > > > > > wrote:
> > > > > > > > > >>
> > > > > > > > > >> > 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

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Manikumar]

; > > >> try {
> > > > > > > > >> GUARD.methodCallInProgress(this);
> > > > > > > > >> return supplier.get();
> > > > > > > > >> } finally {
> > > > > > > > >> GUARD.clearMethodCallInProgress(this);
> > > > > > > > >> }
> > > > > > > > >> }
> > > > > > > > >> }
> > > > > > > > >> }
> > > > > > > > >>
> > > > > > > > >> Guard GUARD = new Guard();
> > > > > > > > >>
> > > > > > > > >> void configure(Map configs, boolean isKey);
> > > > > > > > >>
> > > > > > > > >> default T deserialize(String topic, byte[] data) {
> > > > > > > > >> return GUARD.guard(() -> deserialize(topic, null,
> > > > data));
> > > > > > > > >> }
> > > > > > > > >>
> > > > > > > > >> default T deserialize(String topic, Headers headers,
> > > byte[]
> > > > > > data)
> > > > > > > {
> > > > > > > > >> return GUARD.guard(() -> deserialize(topic,
> data));
> > > > > > > > >> }
> > > > > > > > >>
> > > > > > > > >> @Override
> > > > > > > > >> void close();
> > > > > > > > >> }
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >> Cheers,
> > > > > > > > >> Viktor
> > > > > > > > >>
> > > > > > > > >> On Thu, Aug 23, 2018 at 3:50 PM Ismael Juma <
> > > ism...@juma.me.uk>
> > > > > > > wrote:
> > > > > > > > >>
> > > > > > > > >> > 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
> > > > > > > > >> > >> > > > >
> > > > > > > > >> > >> > > >
> > > > > > > > >> > >> > >
> > > > > > > > >> > >> >
> > > > > > > > >> > >>
> > > > > > > > >> > >
> > > > > > > > >> >
> > > > > > > > >>
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Jason Gustafson]

t; >> }
> > > > > > > >>
> > > > > > > >> @Override
> > > > > > > >> void close();
> > > > > > > >> }
> > > > > > > >>
> > > > > > > >>
> > > > > > > >> Cheers,
> > > > > > > >> Viktor
> > > > > > > >>
> > > > > > > >> On Thu, Aug 23, 2018 at 3:50 PM Ismael Juma <
> > ism...@juma.me.uk>
> > > > > > wrote:
> > > > > > > >>
> > > > > > > >> > 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
> > > > > > > >> > >> > > > >
> > > > > > > >> > >> > > >
> > > > > > > >> > >> > >
> > > > > > > >> > >> >
> > > > > > > >> > >>
> > > > > > > >> > >
> > > > > > > >> >
> > > > > > > >>
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Viktor Somogyi-Vass]

t; me
> > > > > an
> > > > > > >> overshoot. So just for the sake of completeness I'll copy it
> > here.
> > > > :)
> > > > > > >>
> > > > > > >> public interface Deserializer extends Closeable {
> > > > > > >>
> > > > > > >> class Guard {
> > > > > > >>
> > > > > > >> private Set objects =
> > > > Collections.synchronizedSet(new
> > > > > > >> HashSet<>()); // might as well use concurrent hashmap
> > > > > > >>
> > > > > > >> private void methodCallInProgress(Object x) {
> > > > > > >> objects.add(x);
> > > > > > >> }
> > > > > > >>
> > > > > > >> private boolean isMethodCallInProgress(Object x) {
> > > > > > >> return objects.contains(x);
> > > > > > >> }
> > > > > > >>
> > > > > > >> private void clearMethodCallInProgress(Object x) {
> > > > > > >> objects.remove(x);
> > > > > > >> }
> > > > > > >>
> > > > > > >> private  T guard(Supplier supplier) {
> > > > > > >> if (GUARD.isMethodCallInProgress(this)) {
> > > > > > >> throw new IllegalStateException("You must
> > > implement
> > > > > one
> > > > > > of
> > > > > > >> the deserialize methods");
> > > > > > >> } else {
> > > > > > >> try {
> > > > > > >> GUARD.methodCallInProgress(this);
> > > > > > >> return supplier.get();
> > > > > > >> } finally {
> > > > > > >> GUARD.clearMethodCallInProgress(this);
> > > > > > >> }
> > > > > > >> }
> > > > > > >> }
> > > > > > >> }
> > > > > > >>
> > > > > > >> Guard GUARD = new Guard();
> > > > > > >>
> > > > > > >> void configure(Map configs, boolean isKey);
> > > > > > >>
> > > > > > >> default T deserialize(String topic, byte[] data) {
> > > > > > >> return GUARD.guard(() -> deserialize(topic, null,
> > data));
> > > > > > >> }
> > > > > > >>
> > > > > > >> default T deserialize(String topic, Headers headers,
> byte[]
> > > > data)
> > > > > {
> > > > > > >> return GUARD.guard(() -> deserialize(topic, data));
> > > > > > >> }
> > > > > > >>
> > > > > > >> @Override
> > > > > > >> void close();
> > > > > > >> }
> > > > > > >>
> > > > > > >>
> > > > > > >> Cheers,
> > > > > > >> Viktor
> > > > > > >>
> > > > > > >> On Thu, Aug 23, 2018 at 3:50 PM Ismael Juma <
> ism...@juma.me.uk>
> > > > > wrote:
> > > > > > >>
> > > > > > >> > 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
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Ismael Juma]

gt; > > > >> objects.remove(x);
> > > > > >> }
> > > > > >>
> > > > > >> private  T guard(Supplier supplier) {
> > > > > >> if (GUARD.isMethodCallInProgress(this)) {
> > > > > >> throw new IllegalStateException("You must
> > implement
> > > > one
> > > > > of
> > > > > >> the deserialize methods");
> > > > > >> } else {
> > > > > >> try {
> > > > > >> GUARD.methodCallInProgress(this);
> > > > > >> return supplier.get();
> > > > > >> } finally {
> > > > > >> GUARD.clearMethodCallInProgress(this);
> > > > > >> }
> > > > > >> }
> > > > > >> }
> > > > > >> }
> > > > > >>
> > > > > >> Guard GUARD = new Guard();
> > > > > >>
> > > > > >> void configure(Map configs, boolean isKey);
> > > > > >>
> > > > > >> default T deserialize(String topic, byte[] data) {
> > > > > >> return GUARD.guard(() -> deserialize(topic, null,
> data));
> > > > > >> }
> > > > > >>
> > > > > >> default T deserialize(String topic, Headers headers, byte[]
> > > data)
> > > > {
> > > > > >> return GUARD.guard(() -> deserialize(topic, data));
> > > > > >> }
> > > > > >>
> > > > > >> @Override
> > > > > >> void close();
> > > > > >> }
> > > > > >>
> > > > > >>
> > > > > >> Cheers,
> > > > > >> Viktor
> > > > > >>
> > > > > >> On Thu, Aug 23, 2018 at 3:50 PM Ismael Juma 
> > > > wrote:
> > > > > >>
> > > > > >> > 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, 

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Viktor Somogyi-Vass]

ic, byte[] data) {
> > > > >> return GUARD.guard(() -> deserialize(topic, null, data));
> > > > >> }
> > > > >>
> > > > >> default T deserialize(String topic, Headers headers, byte[]
> > data)
> > > {
> > > > >> return GUARD.guard(() -> deserialize(topic, data));
> > > > >> }
> > > > >>
> > > > >> @Override
> > > > >> void close();
> > > > >> }
> > > > >>
> > > > >>
> > > > >> Cheers,
> > > > >> Viktor
> > > > >>
> > > > >> On Thu, Aug 23, 2018 at 3:50 PM Ismael Juma 
> > > wrote:
> > > > >>
> > > > >> > 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 
> > > > 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
> > > > >> > >> > > > >
> > > > >> > >> > > >
> > > > >> > >> > >
> > > > >> > >> >
> > > > >> > >>
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Ismael Juma]

>
> > > >> > > 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
> > > >> > >> > > > >
> > > >> > >> > > >
> > > >> > >> > >
> > > >> > >> >
> > > >> > >>
> > > >> > >
> > > >> >
> > > >>
> > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Jason Gustafson]

gt; > >> 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  >
> > >> > 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
> > >> > >> > > > >
> > >> > >> > > >
> > >> > >> > >
> > >> > >> >
> > >> > >>
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Viktor Somogyi-Vass]

oseable {
> >>
> >> class Guard {
> >>
> >> private Set objects = Collections.synchronizedSet(new
> >> HashSet<>()); // might as well use concurrent hashmap
> >>
> >> private void methodCallInProgress(Object x) {
> >> objects.add(x);
> >> }
> >>
> >> private boolean isMethodCallInProgress(Object x) {
> >> return objects.contains(x);
> >> }
> >>
> >> private void clearMethodCallInProgress(Object x) {
> >> objects.remove(x);
> >> }
> >>
> >> private  T guard(Supplier supplier) {
> >> if (GUARD.isMethodCallInProgress(this)) {
> >> throw new IllegalStateException("You must implement one
> of
> >> the deserialize methods");
> >> } else {
> >> try {
> >> GUARD.methodCallInProgress(this);
> >> return supplier.get();
> >> } finally {
> >> GUARD.clearMethodCallInProgress(this);
> >> }
> >> }
> >> }
> >> }
> >>
> >> Guard GUARD = new Guard();
> >>
> >> void configure(Map configs, boolean isKey);
> >>
> >> default T deserialize(String topic, byte[] data) {
> >> return GUARD.guard(() -> deserialize(topic, null, data));
> >> }
> >>
> >> default T deserialize(String topic, Headers headers, byte[] data) {
> >> return GUARD.guard(() -> deserialize(topic, data));
> >> }
> >>
> >> @Override
> >> void close();
> >> }
> >>
> >>
> >> Cheers,
> >> Viktor
> >>
> >> On Thu, Aug 23, 2018 at 3:50 PM Ismael Juma  wrote:
> >>
> >> > 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 
> 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
> >> > >>
&g

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Jason Gustafson]

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 
>> > >> 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 
>> > 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
>> > >> > > > >
>> > >> > > >
>> > >> > >
>> > >> >
>> > >>
>> > >
>> >
>>
>
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Jason Gustafson]

> > >>
> > >> On Wed, Aug 22, 2018 at 6:11 PM Jason Gustafson 
> > >> 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 
> > 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
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Viktor Somogyi-Vass]

Hi Ismael,

Regarding the deprecation of the 2 parameter method: should we do this with
the Serializer interface as well?

I've updated the "Rejected Alternatives" with a few.
I've added this circular reference one too but actually there's a way
(pretty heavyweight) by adding a guard class that prevents recursive
invocation of either methods. I've tried this out but it seems to me an
overshoot. So just for the sake of completeness I'll copy it here. :)

public interface Deserializer extends Closeable {

class Guard {

private Set objects = Collections.synchronizedSet(new
HashSet<>()); // might as well use concurrent hashmap

private void methodCallInProgress(Object x) {
objects.add(x);
}

private boolean isMethodCallInProgress(Object x) {
return objects.contains(x);
}

private void clearMethodCallInProgress(Object x) {
objects.remove(x);
}

private  T guard(Supplier supplier) {
if (GUARD.isMethodCallInProgress(this)) {
throw new IllegalStateException("You must implement one of
the deserialize methods");
} else {
try {
GUARD.methodCallInProgress(this);
return supplier.get();
} finally {
GUARD.clearMethodCallInProgress(this);
}
}
}
}

Guard GUARD = new Guard();

void configure(Map configs, boolean isKey);

default T deserialize(String topic, byte[] data) {
return GUARD.guard(() -> deserialize(topic, null, data));
}

default T deserialize(String topic, Headers headers, byte[] data) {
return GUARD.guard(() -> deserialize(topic, data));
}

@Override
void close();
}


Cheers,
Viktor

On Thu, Aug 23, 2018 at 3:50 PM Ismael Juma  wrote:

> 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  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 
> >> 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 W

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Ismael Juma]

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  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 
>> 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  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
>> > > > >
>> > > >
>> > >
>> >
>>
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Ismael Juma]

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 
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 
> 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  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
> > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Viktor Somogyi-Vass]

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  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 
> wrote:
>
> > +1
> >
> > On Wed, Aug 22, 2018 at 4:45 PM, Ted Yu  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
> > > >
> > >
> >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Jason Gustafson]

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 
wrote:

> +1
>
> On Wed, Aug 22, 2018 at 4:45 PM, Ted Yu  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
> > >
> >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Satish Duggana]

+1

On Wed, Aug 22, 2018 at 4:45 PM, Ted Yu  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
> >
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Ted Yu]

+1
 Original message From: Kamal Chandraprakash 
 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 
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
>

Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Kamal Chandraprakash]

+1

Thanks for the KIP!

On Wed, Aug 22, 2018 at 2:48 PM Viktor Somogyi-Vass 
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
>

[VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer

[Viktor Somogyi-Vass]

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