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