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