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