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