Sounds good!
On Tue, May 14, 2019 at 9:21 AM Development <d...@yeralin.net> wrote:
>
> Hey,
>
> I think it the proposal is finalized, no one raised any concerns. Shall we
> call it for a [VOTE]?
>
> Best,
> Daniyar Yeralin
>
> > On May 10, 2019, at 10:17 AM, John Roesler <j...@confluent.io> wrote:
> >
> > Good observation, Daniyar.
> >
> > Maybe we should just not implement support for serdeFrom.
> >
> > We can always add it later, but I think you're right, we need some
> > kind of more sophisticated support, or at least a second argument for
> > the inner class.
> >
> > For now, it seems like most use cases would be satisfied without
> > serdeFrom(...List...)
> >
> > -John
> >
> > On Fri, May 10, 2019 at 8:57 AM Development <d...@yeralin.net> wrote:
> >>
> >> Hi,
> >>
> >> I was trying to add some test cases for the list serde, and it led me to
> >> this class `org.apache.kafka.common.serialization.SerializationTest`. I
> >> saw that it relies on method
> >> `org.apache.kafka.common.serialization.serdeFrom(Class<T> type)`
> >>
> >> Now, I’m not sure how to adapt List<T> serde for this method, since it
> >> will be a “nested class”. What is the best approach in this case?
> >>
> >> I remember that in Jackson for example, one uses a TypeFactory, and
> >> constructs “collectionType” of two classes. For example,
> >> `constructCollectionType(List.class, String.class).getClass()`. I don’t
> >> think it applies here.
> >>
> >> Any ideas?
> >>
> >> Best,
> >> Daniyar Yeralin
> >>
> >>> On May 9, 2019, at 2:10 PM, Development <d...@yeralin.net> wrote:
> >>>
> >>> Hey Sophie,
> >>>
> >>> Thank you for your input. I think I’d rather finish this KIP as is, and
> >>> then open a new one for the Collections (if everyone agrees). I don’t
> >>> want to extend the current KIP-466, since most of the work is already
> >>> done for it.
> >>>
> >>> Meanwhile, I’ll start adding some test cases for this new list serde
> >>> since this discussion seems to be approaching its logical end.
> >>>
> >>> Best,
> >>> Daniyar Yeralin
> >>>
> >>>> On May 9, 2019, at 1:35 PM, Sophie Blee-Goldman <sop...@confluent.io>
> >>>> wrote:
> >>>>
> >>>> Good point about serdes for other Collections. On the one hand I'd guess
> >>>> that non-List Collections are probably relatively rare in practice (if
> >>>> anyone disagrees please correct me!) but on the other hand, a) even if
> >>>> just
> >>>> a small number of people benefit I think it's worth the extra effort and
> >>>> b)
> >>>> if we do end up needing/wanting them in the future it would save us a KIP
> >>>> to just add them now. Personally I feel it would make sense to expand the
> >>>> scope of this KIP a bit to include all Collections as a logical unit, but
> >>>> the ROI could be low..
> >>>>
> >>>> (I know of at least one instance in the unit tests where a Set serde
> >>>> could
> >>>> be useful, and there may be more)
> >>>>
> >>>> On Thu, May 9, 2019 at 7:27 AM Development <d...@yeralin.net> wrote:
> >>>>
> >>>>> Hey,
> >>>>>
> >>>>> I don’t see any replies. Seems like this proposal can be finalized and
> >>>>> called for a vote?
> >>>>>
> >>>>> Also I’ve been thinking. Do we need more serdes for other Collections?
> >>>>> Like queue or set for example
> >>>>>
> >>>>> Best,
> >>>>> Daniyar Yeralin
> >>>>>
> >>>>>> On May 8, 2019, at 2:28 PM, John Roesler <j...@confluent.io> wrote:
> >>>>>>
> >>>>>> Hi Daniyar,
> >>>>>>
> >>>>>> No worries about the procedural stuff. Prior experience with KIPs is
> >>>>>> not required :)
> >>>>>>
> >>>>>> I was just trying to help you propose this stuff in a way that the
> >>>>>> others will find easy to review.
> >>>>>>
> >>>>>> Thanks for updating the KIP. Thanks to the others for helping out with
> >>>>>> the syntax.
> >>>>>>
> >>>>>> Given these updates, I'm curious if anyone else has feedback about
> >>>>>> this proposal. Personally, I think it sounds fine!
> >>>>>>
> >>>>>> -John
> >>>>>>
> >>>>>> On Wed, May 8, 2019 at 1:01 PM Development <d...@yeralin.net> wrote:
> >>>>>>>
> >>>>>>> Hey,
> >>>>>>>
> >>>>>>> That worked! I certainly lack Java generics knowledge. Thanks for the
> >>>>> snippet. I’ll update KIP again.
> >>>>>>>
> >>>>>>> Best,
> >>>>>>> Daniyar Yeralin
> >>>>>>>
> >>>>>>>> On May 8, 2019, at 1:39 PM, Chris Egerton <chr...@confluent.io>
> >>>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> Hi Daniyar,
> >>>>>>>>
> >>>>>>>> I think you may want to tweak your syntax a little:
> >>>>>>>>
> >>>>>>>> public static <T> Serde<List<T>> List(Serde<T> innerSerde) {
> >>>>>>>> return new ListSerde<T>(innerSerde);
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> Does that work?
> >>>>>>>>
> >>>>>>>> Cheers,
> >>>>>>>>
> >>>>>>>> Chris
> >>>>>>>>
> >>>>>>>> On Wed, May 8, 2019 at 10:29 AM Development <d...@yeralin.net
> >>>>>>>> <mailto:
> >>>>> d...@yeralin.net>> wrote:
> >>>>>>>> Hi John,
> >>>>>>>>
> >>>>>>>> I updated JIRA and KIP.
> >>>>>>>>
> >>>>>>>> I didn’t know about the process, and created PR before I knew about
> >>>>> KIPs :)
> >>>>>>>>
> >>>>>>>> As per static declaration, I don’t think Java allows that:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Best,
> >>>>>>>> Daniyar Yeralin
> >>>>>>>>
> >>>>>>>>> On May 7, 2019, at 2:22 PM, John Roesler <j...@confluent.io <mailto:
> >>>>> j...@confluent.io>> wrote:
> >>>>>>>>>
> >>>>>>>>> Thanks for that update. Do you mind making changes primarily on the
> >>>>>>>>> KIP document ? (
> >>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-466%3A+Add+support+for+List%3CT%3E+serialization+and+deserialization
> >>>>> <
> >>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-466%3A+Add+support+for+List%3CT%3E+serialization+and+deserialization
> >>>>>> )
> >>>>>>>>>
> >>>>>>>>> This is the design document that we have to agree on and vote for,
> >>>>>>>>> the
> >>>>>>>>> PR comes later. It can be nice to have an implementation to look at,
> >>>>>>>>> but the KIP is the main artifact for this discussion.
> >>>>>>>>>
> >>>>>>>>> With this in mind, it will help get more reviewers to look at it if
> >>>>>>>>> you can tidy up the KIP document so that it stands on its own.
> >>>>>>>>> People
> >>>>>>>>> shouldn't have to look at any other document to understand the
> >>>>>>>>> motivation of the proposal, and they shouldn't have to look at a PR
> >>>>>>>>> to
> >>>>>>>>> see what the public API will look like. If it helps, you can take a
> >>>>>>>>> look at some other recent KIPs.
> >>>>>>>>>
> >>>>>>>>> Given that the list serde needs an inner serde, I agree you can't
> >>>>>>>>> have
> >>>>>>>>> a zero-argument static factory method for it, but it seems you could
> >>>>>>>>> still have a static method:
> >>>>>>>>> `public static Serde<List<T>> List(Serde<T> innerSerde)`.
> >>>>>>>>>
> >>>>>>>>> Thoughts?
> >>>>>>>>>
> >>>>>>>>> On Tue, May 7, 2019 at 12:18 PM Development <d...@yeralin.net
> >>>>>>>>> <mailto:
> >>>>> d...@yeralin.net>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Absolutely agree. Already pushed another commit to remove
> >>>>>>>>>> comparator
> >>>>> argument: https://github.com/apache/kafka/pull/6592 <
> >>>>> https://github.com/apache/kafka/pull/6592> <
> >>>>> https://github.com/apache/kafka/pull/6592 <
> >>>>> https://github.com/apache/kafka/pull/6592>>
> >>>>>>>>>>
> >>>>>>>>>> Thank you for your input John! I really appreciate it.
> >>>>>>>>>>
> >>>>>>>>>> What about this point I made:
> >>>>>>>>>>
> >>>>>>>>>> 1. Since type for List serde needs to be declared before hand, I
> >>>>> could not create a static method for List Serde under
> >>>>> org.apache.kafka.common.serialization.Serdes. I addressed it in the KIP:
> >>>>>>>>>> P.S. Static method corresponding to ListSerde under
> >>>>> org.apache.kafka.common.serialization.Serdes (something like static
> >>>>> public
> >>>>> Serde<List<T>> List() {...}
> >>>>> inorg.apache.kafka.common.serialization.Serdes)
> >>>>> class cannot be added because type needs to be defined beforehand.
> >>>>> That's
> >>>>> why one needs to create List Serde in the following fashion:
> >>>>>>>>>> new Serdes.ListSerde<String>(Serdes.String(),
> >>>>> Comparator.comparing(String::length));
> >>>>>>>>>> (can possibly be simplified by declaring import static
> >>>>> org.apache.kafka.common.serialization.Serdes.ListSerde)
> >>>>>>>>>>
> >>>>>>>>>>> On May 7, 2019, at 11:50 AM, John Roesler <j...@confluent.io
> >>>>> <mailto:j...@confluent.io>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks for the reply Daniyar,
> >>>>>>>>>>>
> >>>>>>>>>>> That makes much more sense! I thought I must be missing something,
> >>>>> but I
> >>>>>>>>>>> couldn't for the life of me figure it out.
> >>>>>>>>>>>
> >>>>>>>>>>> What do you think about just taking an argument, instead of for a
> >>>>>>>>>>> Comparator, for the Serde of the inner type? That way, the user
> >>>>>>>>>>> can
> >>>>> control
> >>>>>>>>>>> how exactly the inner data gets serialized, while also bounding
> >>>>>>>>>>> the
> >>>>> generic
> >>>>>>>>>>> parameter properly. As for the order, since the list is already
> >>>>>>>>>>> in a
> >>>>>>>>>>> specific order, which the user themselves controls, it doesn't
> >>>>>>>>>>> seem
> >>>>>>>>>>> strictly necessary to offer an option to sort the data during
> >>>>> serialization.
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks,
> >>>>>>>>>>> -John
> >>>>>>>>>>>
> >>>>>>>>>>> On Mon, May 6, 2019 at 8:47 PM Development <d...@yeralin.net
> >>>>> <mailto:d...@yeralin.net>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> Hi John,
> >>>>>>>>>>>>
> >>>>>>>>>>>> I’m really sorry for the confusion. I cloned that JIRA ticket
> >>>>>>>>>>>> from
> >>>>> an old
> >>>>>>>>>>>> one about introducing UUID Serde, and I guess was too hasty while
> >>>>> editing
> >>>>>>>>>>>> the copy to notice the mistake. Just edited the ticket. Sorry for
> >>>>> any
> >>>>>>>>>>>> inconvenience .
> >>>>>>>>>>>>
> >>>>>>>>>>>> As per comparator, I agree. Let’s make user be responsible for
> >>>>>>>>>>>> implementing comparable interface. I was just thinking to make
> >>>>>>>>>>>> the
> >>>>> serde a
> >>>>>>>>>>>> little more flexible (i.e. let user decide in which order records
> >>>>> is going
> >>>>>>>>>>>> to be inserted into a change log topic).
> >>>>>>>>>>>>
> >>>>>>>>>>>> Thank you!
> >>>>>>>>>>>>
> >>>>>>>>>>>> Best,
> >>>>>>>>>>>> Daniyar Yeralin
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> On May 6, 2019, at 5:37 PM, John Roesler <j...@confluent.io
> >>>>> <mailto:j...@confluent.io>> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Hi Daniyar,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Thanks for the proposal!
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> If I understand the point about the comparator, is it just to
> >>>>> capture the
> >>>>>>>>>>>>> generic type parameter? If so, then anything that implements a
> >>>>> known
> >>>>>>>>>>>>> interface would work just as well, right? I've been considering
> >>>>> adding
> >>>>>>>>>>>>> something like the Jackson TypeReference (or similar classes in
> >>>>> many
> >>>>>>>>>>>> other
> >>>>>>>>>>>>> projects). Would this be a good time to do it?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Note that it's not necessary to actually require that the
> >>>>> captured type
> >>>>>>>>>>>> is
> >>>>>>>>>>>>> Comparable (as this proposal currently does), it's just a way to
> >>>>> make
> >>>>>>>>>>>> sure
> >>>>>>>>>>>>> there is some method that makes use of the generic type
> >>>>> parameter, to
> >>>>>>>>>>>> force
> >>>>>>>>>>>>> the compiler to capture the type.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Just to make sure I understand the motivation... You expressed a
> >>>>> desire
> >>>>>>>>>>>> to
> >>>>>>>>>>>>> be able to serialize UUIDs, which I didn't follow, since there
> >>>>>>>>>>>>> is
> >>>>> a
> >>>>>>>>>>>>> built-in UUID serde:
> >>>>> org.apache.kafka.common.serialization.Serdes#UUID,
> >>>>>>>>>>>> and
> >>>>>>>>>>>>> also, a UUID isn't a List. Did you mean that you need to use
> >>>>> *lists of*
> >>>>>>>>>>>>> UUIDs?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>> -John
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Mon, May 6, 2019 at 11:49 AM Development <d...@yeralin.net
> >>>>> <mailto:d...@yeralin.net>> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> Hello,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Starting a discussion for KIP-466 adding support for List
> >>>>>>>>>>>>>> Serde.
> >>>>> PR is
> >>>>>>>>>>>>>> created under https://github.com/apache/kafka/pull/6592 <
> >>>>> https://github.com/apache/kafka/pull/6592> <
> >>>>>>>>>>>>>> https://github.com/apache/kafka/pull/6592 <
> >>>>> https://github.com/apache/kafka/pull/6592>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> There are two topics I would like to discuss:
> >>>>>>>>>>>>>> 1. Since type for List serve needs to be declared before hand,
> >>>>>>>>>>>>>> I
> >>>>> could
> >>>>>>>>>>>> not
> >>>>>>>>>>>>>> create a static method for List Serde under
> >>>>>>>>>>>>>> org.apache.kafka.common.serialization.Serdes. I addressed it in
> >>>>> the KIP:
> >>>>>>>>>>>>>> P.S. Static method corresponding to ListSerde under
> >>>>>>>>>>>>>> org.apache.kafka.common.serialization.Serdes (something like
> >>>>> static
> >>>>>>>>>>>> public
> >>>>>>>>>>>>>> Serde<List<T>> List() {...}
> >>>>>>>>>>>> inorg.apache.kafka.common.serialization.Serdes)
> >>>>>>>>>>>>>> class cannot be added because type needs to be defined
> >>>>> beforehand.
> >>>>>>>>>>>> That's
> >>>>>>>>>>>>>> why one needs to create List Serde in the following fashion:
> >>>>>>>>>>>>>> new Serdes.ListSerde<String>(Serdes.String(),
> >>>>>>>>>>>>>> Comparator.comparing(String::length));
> >>>>>>>>>>>>>> (can possibly be simplified by declaring import static
> >>>>>>>>>>>>>> org.apache.kafka.common.serialization.Serdes.ListSerde)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> 2. @miguno Michael G. Noll <https://github.com/miguno <
> >>>>> https://github.com/miguno>> is questioning
> >>>>>>>>>>>>>> whether I need to pass a comparator to ListDeserializer. This
> >>>>> certainly
> >>>>>>>>>>>> is
> >>>>>>>>>>>>>> not required. Feel free to add your input:
> >>>>>>>>>>>>>> https://github.com/apache/kafka/pull/6592#discussion_r281152067
> >>>>> <https://github.com/apache/kafka/pull/6592#discussion_r281152067>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Thank you!
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Best,
> >>>>>>>>>>>>>> Daniyar Yeralin
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On May 6, 2019, at 11:59 AM, Daniyar Yeralin (JIRA) <
> >>>>> j...@apache.org <mailto:j...@apache.org>>
> >>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Daniyar Yeralin created KAFKA-8326:
> >>>>>>>>>>>>>>> --------------------------------------
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Summary: Add List<T> Serde
> >>>>>>>>>>>>>>> Key: KAFKA-8326
> >>>>>>>>>>>>>>> URL:
> >>>>> https://issues.apache.org/jira/browse/KAFKA-8326 <
> >>>>> https://issues.apache.org/jira/browse/KAFKA-8326>
> >>>>>>>>>>>>>>> Project: Kafka
> >>>>>>>>>>>>>>> Issue Type: Improvement
> >>>>>>>>>>>>>>> Components: clients, streams
> >>>>>>>>>>>>>>> Reporter: Daniyar Yeralin
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I propose adding serializers and deserializers for the
> >>>>> java.util.List
> >>>>>>>>>>>>>> class.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I have many use cases where I want to set the key of a Kafka
> >>>>> message to
> >>>>>>>>>>>>>> be a UUID. Currently, I need to turn UUIDs into strings or byte
> >>>>> arrays
> >>>>>>>>>>>> and
> >>>>>>>>>>>>>> use their associated Serdes, but it would be more convenient to
> >>>>>>>>>>>> serialize
> >>>>>>>>>>>>>> and deserialize UUIDs directly.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I believe there are many use cases where one would want to
> >>>>>>>>>>>>>>> have
> >>>>> a List
> >>>>>>>>>>>>>> serde. Ex. [
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>> https://stackoverflow.com/questions/41427174/aggregate-java-objects-in-a-list-with-kafka-streams-dsl-windows
> >>>>> <
> >>>>> https://stackoverflow.com/questions/41427174/aggregate-java-objects-in-a-list-with-kafka-streams-dsl-windows
> >>>>>>
> >>>>>>>>>>>> ],
> >>>>>>>>>>>>>> [
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>> https://stackoverflow.com/questions/46365884/issue-with-arraylist-serde-in-kafka-streams-api
> >>>>> <
> >>>>> https://stackoverflow.com/questions/46365884/issue-with-arraylist-serde-in-kafka-streams-api
> >>>>>>
> >>>>>>>>>>>>>> ]
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> KIP Link: [
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-466%3A+Add+support+for+List%3CT%3E+serialization+and+deserialization
> >>>>> <
> >>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-466%3A+Add+support+for+List%3CT%3E+serialization+and+deserialization
> >>>>>>
> >>>>>>>>>>>>>> ]
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> --
> >>>>>>>>>>>>>>> This message was sent by Atlassian JIRA
> >>>>>>>>>>>>>>> (v7.6.3#76005)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>
> >>
>
