kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Viktor Somogyi-Vass <viktorsomo...@gmail.com>
Subject Re: [VOTE] KIP-336: Consolidate ExtendedSerializer/Serializer and ExtendedDeserializer/Deserializer
Date Mon, 27 Aug 2018 14:56:43 GMT
Thanks guys, I've updated my KIP with this info (so to keep solution #1).
If you find it good enough, please vote as well or let me know if you think
something is missing.

On Sat, Aug 25, 2018 at 1:14 AM Ismael Juma <ismael@juma.me.uk> wrote:

> I'm OK with 1 too. It makes me a bit sad that we don't have a path for
> removing the method without headers, but it seems like the simplest and
> least confusing option (I am assuming that headers are not needed in the
> serializers in the common case).
>
> Ismael
>
> On Fri, Aug 24, 2018 at 2:42 PM Jason Gustafson <jason@confluent.io>
> wrote:
>
> > Hey Viktor,
> >
> > Good summary. I agree that option 1) seems like the simplest choice and,
> as
> > you note, we can always add the default implementation later. I'll leave
> > Ismael to make a case for the circular forwarding approach ;)
> >
> > -Jason
> >
> > On Fri, Aug 24, 2018 at 3:02 AM, Viktor Somogyi-Vass <
> > viktorsomogyi@gmail.com> wrote:
> >
> > > I think in the first draft I didn't provide an implementation for them
> as
> > > it seemed very simple and straightforward. I looked up a couple of
> > > implementations of the ExtendedSerializers on github and the general
> > > behavior seems to be that they delegate to the 2 argument (headerless)
> > > method:
> > >
> > > https://github.com/khoitnm/practice-spring-kafka-grpc/blob/
> > > a6fc9b3395762c4889807baedd822f4653d5dcdd/kafka-common/src/
> > > main/java/org/tnmk/common/kafka/serialization/protobuf/
> > > ProtobufSerializer.java
> > > https://github.com/hong-zhu/nxgen/blob/5cf1427d4e1a8f5c7fab47955af32a
> > > 0d4f4873af/nxgen-kafka-client/src/main/java/nxgen/kafka/
> > > client/event/serdes/EventSerializer.java
> > > https://github.com/jerry-jx/spring-kafka/blob/
> > > ac323ec5b8b9a0ca975db2f7322ff6878fce481a/spring-kafka/src/
> > >
> >
> main/java/org/springframework/kafka/support/serializer/JsonSerializer.java
> > > https://github.com/enzobonggio/nonblocking-kafka/blob/
> > > bc1a379b2d9593b46cf9604063bc5b38e2785d19/src/main/java/com/
> > > example/kafka/producer/CustomJsonSerializer.java
> > >
> > > Of course 4 example is not representative but it shows that these users
> > > usually delegate to the "headerless" (2 argument) method. I've tried to
> > > look it up on other code search sites but haven't had much luck so far.
> > > Given these examples and the way they implement them I'd say it's more
> > > common to delegate to the headerless method, that's why I think it's a
> > good
> > > approach for us too. Now having a default implementation for that is
> > again
> > > a good question. I think current use cases wouldn't change in either
> case
> > > (unless we deprecate the headerless one).
> > > For the new use cases it depends what do we want to propagate going
> > > forward. Do we want only one method to exist or two? As Ismael
> > highlighted
> > > it might be confusing if we have 2 methods, both with default
> > > implementation and in this case we want to push the 3 argument one for
> > > users.
> > >
> > > So I see three possible ways:
> > > 1.) Don't provide a default implementation for the headerless method.
> > This
> > > supports the current implementations and encourages the delegation
> style
> > in
> > > future implementations. This might be the simplest option.
> > > 2.) Provide a default implementation for the headerless method. This
> > would
> > > be a bit confusing, so we'd likely push the use of the 3 parameter
> method
> > > and deprecate the headerless. This would however further litter the
> code
> > > base with deprecation warnings as we're using the headerless method in
> a
> > > lot of places (think of the current serializers/deserializers). So in
> > this
> > > case we would want to clean up the code base a little where we can and
> > may
> > > remove the headerless method entirely in Kafka 3. But they would hang
> > > around until that point. I think in this case the implementation for
> the
> > > headerless is a detail question as that is deprecated so we don't
> expect
> > > new implementations to use that method.
> > > If we decide to move this way, we have explored two options so far:
> > > returning null / empty array or throwing exceptions. (And I honestly
> > > started to like the latter as calling that with no real implementation
> is
> > > really a programming error.)
> > > 3.) We can do it in multiple steps. In the first step we do 1 and later
> > 2.
> > > I think it would also make sense as the Kafka code base heavily uses
> the
> > > headerless method still (think of the existing
> serializers/deserializers)
> > > and it would give us time to eliminate/change those use cases.
> > >
> > > Cheers,
> > > Viktor
> > >
> > > On Thu, Aug 23, 2018 at 11:55 PM Jason Gustafson <jason@confluent.io>
> > > wrote:
> > >
> > > > To clarify, what I am suggesting is to only remove the default
> > > > implementation for these methods. So users would be required to
> > implement
> > > > serialize(topic, data) and deserialize(topic, data).
> > > >
> > > > -Jason
> > > >
> > > > On Thu, Aug 23, 2018 at 1:48 PM, Jason Gustafson <jason@confluent.io
> >
> > > > wrote:
> > > >
> > > > > 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 <
> > > > > viktorsomogyi@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<T> extends Closeable {
> > > > >>
> > > > >>     class Guard {
> > > > >>
> > > > >>         private Set<Object> 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> T guard(Supplier<T> 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<String, ?> 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 <ismael@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 <ismael@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
<
> > > > >> > > viktorsomogyi@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
<
> > > > jason@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.duggana@gmail.com>
> > > > >> > >> > wrote:
> > > > >> > >> >
> > > > >> > >> > > +1
> > > > >> > >> > >
> > > > >> > >> > > On Wed, Aug 22, 2018 at 4:45 PM, Ted
Yu <
> > yuzhihong@gmail.com
> > > >
> > > > >> > wrote:
> > > > >> > >> > >
> > > > >> > >> > > > +1
> > > > >> > >> > > > -------- Original message --------From:
Kamal
> > > Chandraprakash
> > > > <
> > > > >> > >> > > > kamal.chandraprakash@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 <
> > > > >> > >> > > > viktorsomogyi@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
> > > > >> > >> > > > >
> > > > >> > >> > > >
> > > > >> > >> > >
> > > > >> > >> >
> > > > >> > >>
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message