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