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 <ja...@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 <
> 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
>> > >> > > > >
>> > >> > > >
>> > >> > >
>> > >> >
>> > >>
>> > >
>> >
>>
>
>
