Hi Ismael,
Yea, I think that's the most painless given the current circumstances,
although a bit weird that we have an interface that doesn't enforce
anything. I guess we can mitigate this on some level with documentation.
However yes, new implementations can freely choose whatever they want to
implement, users will have no need of implementing dummies. One of my
colleague's idea was also that we could build in a check against null for
headers, so it wouldn't be possible to call serialize(bla, null, bla)
during runtime as in this case they should really use the other method.
I also had another idea though. It is a possibility I think that we could
introduce a setHeaders(Headers) method with a default empty implementation
in the Serializer/Deserializer instead of the 3 parameter method and make a
promise that we'll call setHeaders(Headers) right before
serialize/deserialize. That way we could avoid to have the 3 parameter
method (we could deprecate ExtendedSerializer/ExtendedDeserializer
altogether) but we'd possibly introduce some more state into
implementations that use headers which might make users' implementation a
little bit trickier. Not sure if this is better but I thought I'd present
it for the sake of completeness.
So in this case our interface would look like (taking KIP-331 into account
too):
@FunctionalInterface
public interface Serializer<T> extends Closeable {
default void configure(Map<String, ?> configs, boolean isKey){}
byte[] serialize(String topic, T data);
default byte[] setHeaders(Headers headers) {} //
This is the new method
@Override
default void close() {}
}
And the call site would look like (from KafkaProducer):
keySerializer.setHeaders(record.headers());
serializedKey = keySerializer.serialize(record.topic(), record.key());
Do you have any opinions on this?
Viktor
On Tue, Jul 17, 2018 at 10:40 AM Ismael Juma <ism...@juma.me.uk> wrote:
> Hi Viktor,
>
> The approach where all the methods have a default implementation and the
> user chooses to override one of them seems the most appealing to me given
> the current state. It doesn't seem like we give up much in that case, what
> do you think?
>
> Ismael
>
> On Tue, Jul 10, 2018 at 7:15 AM Viktor Somogyi <viktorsomo...@gmail.com>
> wrote:
>
> > Hi Ismael,
> >
> > Well, yes. If we care about headers only then you'd need to add a dummy
> > implementation for the 2 parameter method as well. Although it is not
> > ideal, we're using the Serializer interface everywhere and convert it to
> > extended with ensureExtended(serializer) and delegate to the 2 parameter
> > method inside the wrapper which is returned in ensureExtended. Because of
> > backward compatibility we have to keep delegating but I could perhaps
> add a
> > dummy implementation for the 2 parameter too if you and others think that
> > would be better. In this case though we'd have an interface where all
> > methods are default (given the improvements of 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
> > >)
> > and would have to rethink if this interface should be a
> > @FunctionalInterface.
> > I don't really have a context currently on how the 3 parameter method is
> > used, most the code samples I found on github were using the 2 parameter
> > method. I think I found one instance where the 3 parameter one was used
> but
> > that delegated to the 2 param one :). Have to say though that this
> research
> > is not representative.
> > All in all I think it wouldn't hurt to provide a default implementation
> for
> > the 2 param method too but then we have to give up the
> @FunctionalInterface
> > annotation and we'll end up with an interface with no abstract methods
> but
> > only defaults.
> > What do you think?
> >
> > Cheers,
> > Viktor
> >
> >
> > On Mon, Jul 9, 2018 at 11:02 AM Ismael Juma <ism...@juma.me.uk> wrote:
> >
> > > Thanks for the KIP. It would be helpful to understand the user
> experience
> > > for the case where the implementor uses the headers. It seems like it
> > would
> > > require overriding two methods?
> > >
> > > Ismael
> > >
> > > On Mon, Jul 9, 2018 at 1:50 AM Viktor Somogyi <viktorsomo...@gmail.com
> >
> > > wrote:
> > >
> > > > Hi folks,
> > > >
> > > > I've published KIP-336 which is about consolidating the
> > > > Serializer/Deserializer interfaces.
> > > >
> > > > Basically the story here is when ExtendedSerializer and
> > > > ExtendedDeserializer were added we still supported Java 7 and
> therefore
> > > had
> > > > to use compatible constructs which now seem unnecessary since we
> > dropped
> > > > support for Java 7. Now in this KIP I propose a way to deprecate
> those
> > > > patterns:
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=87298242
> > > >
> > > > I'd be happy to receive some feedback about the KIP I published.
> > > >
> > > > Cheers,
> > > > Viktor
> > > >
> > >
> >
>
