Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Thanks! Glad we are on the same page on how to address the cyclic dependency issue. -Matthias On 7/24/19 8:09 AM, Development wrote: > KIP-466 is updated and new commit is pushed. > > Thank you guys! > >> On Jul 24, 2019, at 10:53 AM, John Roesler wrote: >> >> Ah, thanks for setting me

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

KIP-466 is updated and new commit is pushed. Thank you guys! > On Jul 24, 2019, at 10:53 AM, John Roesler wrote: > > Ah, thanks for setting me straight, Matthias. > > Given the choice between defining the Serde in the streams module (hence it > would not be in the Serdes "menu" class) or

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Ah, thanks for setting me straight, Matthias. Given the choice between defining the Serde in the streams module (hence it would not be in the Serdes "menu" class) or defining the configuration property in CommonClientConfig, I think I'm leaning toward the latter. Really good catch on the

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hey Matthias, Yes, you are totally right, “list.key/value.serializer.type" in ProducerConfigs. Removed! And yes, now StreamsConfig just points to configs stored in CommonClientsConfig. I’ll update the KIP. I think we can continue with voting now. Best, Daniyar Yeralin > On Jul 23, 2019, at

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

>> Just to make sure I understand the problem you're highlighting: >> I guess the difference is that the serializer and deserializer that are >> nested inside the serde also need to be configured? So, by default I'd have >> to specify all six configs when I'm using Streams? That is not the

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hmm, that's a tricky situation. I think Daniyar was on the right track... Producer only cares about serializer configs, and Consumer only cares about deserializer configs. I didn't see the problem with your proposal: ProducerConfig: > list.key/value.serializer.type >

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Bump > On Jul 22, 2019, at 11:26 AM, Development wrote: > > Hey Matthias, > > It looks a little confusing, but I don’t have enough expertise to judge on > the configuration placement. > > If you think, it is fine I’ll go ahead with this approach. > > Best, > Daniyar Yeralin > >> On Jul 19,

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hey Matthias, It looks a little confusing, but I don’t have enough expertise to judge on the configuration placement. If you think, it is fine I’ll go ahead with this approach. Best, Daniyar Yeralin > On Jul 19, 2019, at 5:49 PM, Matthias J. Sax wrote: > > Good point. > > I guess the

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Good point. I guess the simplest solution is, to actually add >> default.list.key/value.serde.type >> default.list.key/value.serde.inner to both `CommonClientConfigs` and `StreamsConfig`. It's not super clean, but I think it's the best we can do. Thoughts? -Matthias On 7/19/19 1:23 PM,

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi Matthias, I agree, ConsumerConfig did not seem like a right place for these configurations. I’ll put them in ProducerConfig, ConsumerConfig, and StreamsConfig. However, I have a question. What should I do in "configure(Map configs, boolean isKey)” methods? Which configurations should I try

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Thanks! One minor question about the configs. The KIP adds three classes, a Serializer, a Deserializer, and a Serde. Hence, would it make sense to add the corresponding configs to `ConsumerConfig`, `ProducerConfig`, and `StreamsConfig` using slightly different names each time? Somethin like

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi, Yes, totally forgot about the statement. KIP-466 is updated. Thank you so much John Roesler, Matthias J. Sax, Sophie Blee-Goldman for your valuable input! I hope I did not cause too much trouble :) I’ll start the vote now. Best, Daniyar Yeralin > On Jul 16, 2019, at 3:17 PM, John

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi Daniyar, Thanks for that update. I took a look, and I think this is in good shape. One note, the statement "New method public static Serde> ListSerde() in org.apache.kafka.common.serialization.Serdes class (infers list implementation and inner serde from config file)" is still present in the

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi, Pushed new changes under my PR: https://github.com/apache/kafka/pull/6592 Feel free to put any comments in there. Best, Daniyar Yeralin > On Jul 15, 2019, at 1:06 PM, Development wrote: > > Hi John, > > I knew I was missing something. Yes,

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi John, I knew I was missing something. Yes, that makes sense now, I removed all `listSerde()` methods, and left empty constructors instead. As per `CommonClientConfigs` I looked at the class, it doesn’t have any properties related to serdes, and that bothers me a little. All properties like

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi all, Regarding the placement, you might as well move the constants to `org.apache.kafka.clients.CommonClientConfigs`, so that the constants and the configs and the code are in the same module. Regarding the constructor... What Matthias said is correct: The serde, serializer, and deserializer

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

One problem though. Since WindowedSerde (Windowed(De)Serializer) are so similar, I’m trying to mimic the implementation of my ListSerde accordingly. I created couple constants under StreamsConfig: And trying to do similar construct: final String propertyName = isKey ?

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi Matthias, Thank you for your input. I updated the KIP, made it a little more readable. I think the configuration parameters strategy is finalized then. Do you have any other questions/concerns regarding this KIP? Meanwhile I’ll start doing appropriate code changes, and commit them under my

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Daniyar, thanks for the update to the KIP. It's in really good shape and well written. About the default constructor question: All Serdes/Serializer/Deserializer classes need a default constructor to create them easily via reflections when specifies in a config. I understand that it is not

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi John, Yes, I do agree. That totally makes sense. The only thing is that it goes against what Matthias suggested earlier: "I think that ... `ListSerde` should have an default constructor and it should be possible to pass in the `Class listClass` information via a configuration. Otherwise,

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Ah, my apologies, I must have just overlooked it. Thanks for the update, too. Just one more super-small question, do we need this variant: > New method public static Serde> ListSerde() in org.apache.kafka.common.serialization.Serdes class (infers list implementation and inner serde from config

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Thanks for the update, Daniyar! In addition to specifying the config interface, can you also specify the Java interface? Namely, if I need to pass an instance of this serde in to the DSL directly, as in Produced, Materialized, etc., what constructor(s) would I have available? Likewise with the

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hey, Finally made updates to the KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-466%3A+Add+support+for+List%3CT%3E+serialization+and+deserialization Sorry for

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Yes, something like this. I did not think about good configuration parameter names yet. I am also not sure if I understand all proposed configs atm. But all configs should be listed and explained in the KIP anyway, and we can discuss further after you have updated the KIP (I can ask more detailed

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Thanks for the update! I think that `ListDeserializer`, `ListSerializer`, and `ListSerde` should have an default constructor and it should be possible to pass in the `Class listClass` information via a configuration. Otherwise, KafkaStreams cannot use it as default serde. For the primitive

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

I made and pushed necessary commits, so we could review the final version under PR https://github.com/apache/kafka/pull/6592 I also need some advice on writing tests for this new serde. So far I only have two test cases (roundtrip and empty payload), I’m not sure if it is enough. Thank y’all

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hey Daniyar, Looks good to me! Thanks for considering it. Thanks, -John On Fri, Jun 21, 2019 at 9:04 AM Development wrote: > Hey John and Matthias, > > Yes, now I see it all. I’m storing lots of redundant information. > Here is my final idea. Yes, now a user should pass a list type. I

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

For encoding the list-type: I see John's point about re-encoding the list-type redundantly. However, I also don't like the idea that the Deserializer returns a fixed type... Maybe it's best allow users to specify the target list type on deserialization via config? Similar for the primitive

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hey Daniyar, Thanks for looking at it! Something like your screenshot is more along the lines of what I was thinking. Sorry, but I didn't follow what you mean, how would that not be "vanilla java"? Unfortunately the deserializer needs more information, though. For example, what if the inner

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hey John, I gave read about TypeReference. It could work for the list serde. However, it is not directly supported: https://github.com/FasterXML/jackson-databind/issues/1490 The only way is to pass an actual class object into the

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi John, Thank you for your input! Yes, my idea looks a little bit over engineered :) I also wanted to see a feedback from Mathias as well since he gave me an idea about storing fixed/variable size entries. Best, Daniyar Yeralin > On Jun 18, 2019, at 6:06 PM, John Roesler wrote: > > Hi

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi Daniyar, That's a very clever solution! One observation is that, now, this is what we might call a polymorphic serde. That is, you're detecting the actual concrete type and then promising to produce the exact same concrete type on read. There are some inherent problems with this approach,

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

bump

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hmm the formatting got removed unfortunately. I’m sorry, it got harder to read my email. > On Jun 12, 2019, at 10:27 AM, Development wrote: > > Hi Matthias, > > Indeed, you are right. I missed your email, I had a problem with my mail > server, so I guess I didn’t receive it. > > 1) Here is

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi Matthias, Indeed, you are right. I missed your email, I had a problem with my mail server, so I guess I didn’t receive it. 1) Here is what I came up with In my ListSerializer.java I have the following: try (final DataOutputStream out = new DataOutputStream(baos)) { // I’m encoding the

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Seems you missed my reply from 31/6. C below: > (1) The current PR suggests to always instantiate an `ArrayList` -- > however, if a user wants to use any other list implementation, they have > no way to specify this. It might be good to either allow users to > specify the list-type on the

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Bump > On May 24, 2019, at 2:09 PM, Development wrote: > > Hey, > > - did we consider to make the return type (ie, ArrayList, vs > LinkesList) configurable or encode it the serialized bytes? > > Not sure about this one. Could you elaborate? > > - atm the size of each element is encoded

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

(1) The current PR suggests to always instantiate an `ArrayList` -- however, if a user wants to use any other list implementation, they have no way to specify this. It might be good to either allow users to specify the list-type on the deserializer, or encode the list type directly in the bytes,

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hey, - did we consider to make the return type (ie, ArrayList, vs LinkesList) configurable or encode it the serialized bytes? Not sure about this one. Could you elaborate? - atm the size of each element is encoded individually; did we consider an optimization for fixed size elements (like Long)

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Thanks for the KIP. I also had a look into the PR and have two follow up question: - did we consider to make the return type (ie, ArrayList, vs LinkesList) configurable or encode it the serialized bytes? - atm the size of each element is encoded individually; did we consider an optimization

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Sounds good! On Tue, May 14, 2019 at 9:21 AM Development 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 wrote: > > > > Good observation, Daniyar.

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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 wrote: > > Good observation, Daniyar. > > Maybe we should just not implement support for serdeFrom. > > We can always add

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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 type)` Now, I’m not sure how to adapt List serde for this

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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 wrote: > > Hi Daniyar, > >

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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 wrote: > > Hi Daniyar, > > I think you may want to tweak your syntax a little: > > public static Serde> List(Serde

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi Daniyar, I think you may want to tweak your syntax a little: public static Serde> List(Serde innerSerde) { return new ListSerde(innerSerde); } Does that work? Cheers, Chris On Wed, May 8, 2019 at 10:29 AM Development wrote: > Hi John, > > I updated JIRA and KIP. > > I didn’t know

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Hi Daniyar, Thanks for the KIP! I had to write my own List serde for testing a while back and this definitely would have saved me some time. Regarding the static declaration, I believe you're missing a between "public static" and the return type "Serde>" -- Java should allow this Cheers,

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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 wrote: > > Thanks for that update. Do you mind making

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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) This is the design document that we have to agree on and vote for, the PR comes later.

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

Absolutely agree. Already pushed another commit to remove comparator argument: 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

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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

Re: [DISCUSS] KIP-466: Add support for List serialization and deserialization

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

[DISCUSS] KIP-466: Add support for List serialization and deserialization

Hello, Starting a discussion for KIP-466 adding support for List Serde. PR is created under 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