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 <matth...@confluent.io> wrote: > > 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, Development wrote: >> 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<String, ?> >> configs, boolean isKey)” methods? Which configurations should I try to >> locate? I was comparing my (de)serializer implementations with >> SessionWindows(De)serializer classes, and they use StreamsConfig class to >> get either StreamsConfig.DEFAULT_WINDOWED_KEY_SERDE_INNER_CLASS : >> StreamsConfig.DEFAULT_WINDOWED_VALUE_SERDE_INNER_CLASS >> >> In my case, as I mentioned earlier, StreamsConfig class is not accessible >> from org.apache.kafka.common.serialization package. So, I can’t utilize it. >> Any suggestions here? >> >> Best, >> Daniyar Yeralin >> >> >>> On Jul 18, 2019, at 8:46 PM, Matthias J. Sax <matth...@confluent.io> wrote: >>> >>> 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 this: >>> >>> ProducerConfig: >>> >>> list.key/value.serializer.type >>> list.key/value.serializer.inner >>> >>> ConsumerConfig: >>> >>> list.key/value.deserializer.type >>> list.key/value.deserializer.inner >>> >>> StreamsConfig: >>> >>> default.list.key/value.serde.type >>> default.list.key/value.serde.inner >>> >>> >>> Adding `d.l.k/v.serde.t/i` to `CommonClientConfigs does not sound right >>> to me. Also note, that it seems better to avoid the `default.` prefix >>> for consumers and producers because there is only one Serializer or >>> Deserializer anyway. Only for Streams, there are multiple and >>> StreamsConfig specifies the default one of an operator does not >>> overwrite it. >>> >>> Thoughts? >>> >>> >>> Also, the KIP should explicitly mention to what classed certain configs >>> are added. Atm, the KIP only list parameter names, but does not state >>> where those are added. >>> >>> >>> -Matthias >>> >>> >>> >>> >>> >>> On 7/16/19 1:11 PM, Development wrote: >>>> 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 Roesler <j...@confluent.io> wrote: >>>>> >>>>> 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 <T> Serde<List<T>> >>>>> ListSerde() in org.apache.kafka.common.serialization.Serdes class >>>>> (infers list implementation and inner serde from config file)" is >>>>> still present in the KIP, although I do it is was removed from the PR. >>>>> >>>>> Once you remove that statement from the KIP, then I think this KIP is >>>>> ready to go up for a vote! Then, we can really review the PR in >>>>> earnest and get this thing merged. >>>>> >>>>> Thanks, >>>>> -john >>>>> >>>>> On Tue, Jul 16, 2019 at 2:05 PM Development <d...@yeralin.net> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> Pushed new changes under my PR: >>>>>> https://github.com/apache/kafka/pull/6592 >>>>>> <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 <d...@yeralin.net> wrote: >>>>>>> >>>>>>> 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 `default.key.serde` `default.windowed.key.serde.*` >>>>>>> are located in StreamsConfig. I don’t want to create a confusion. >>>>>>> What also doesn’t make sense to me is that `WindowedSerdes` and its >>>>>>> (de)serializers are not located in >>>>>>> org.apache.kafka.common.serialization. I guess it kind of makes sense >>>>>>> since windowed serdes are only available for kafka streams, not vice >>>>>>> versa. >>>>>>> >>>>>>> If everyone is okay to put list properties in `CommonClientConfigs` >>>>>>> class, I’ll go ahead and do that then. >>>>>>> >>>>>>> Thank you for your input! >>>>>>> >>>>>>> Best, >>>>>>> Daniyar Yeralin >>>>>>> >>>>>>>> On Jul 15, 2019, at 11:45 AM, John Roesler <j...@confluent.io> wrote: >>>>>>>> >>>>>>>> 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 all need to have zero-arg constructors so >>>>>>>> they can be instantiated reflectively by Kafka. However, the factory >>>>>>>> method you proposed "New method public static <T> Serde<List<T>> >>>>>>>> ListSerde()" is not a constructor, and is not required. It would be >>>>>>>> used purely from the Java interface, but has the drawbacks I listed >>>>>>>> above. This method, not the constructor, is what I proposed to remove >>>>>>>> from the KIP. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> -John >>>>>>>> >>>>>>>> >>>>>>>> On Mon, Jul 15, 2019 at 10:15 AM Development <d...@yeralin.net >>>>>>>> <mailto:d...@yeralin.net>> wrote: >>>>>>>> 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 ? >>>>>>>> StreamsConfig.DEFAULT_LIST_KEY_SERDE_INNER_CLASS : >>>>>>>> StreamsConfig.DEFAULT_LIST_VALUE_SERDE_INNER_CLASS; >>>>>>>> But then found out that StreamsConfig is not accessible from >>>>>>>> org.apache.kafka.common.serialization package while window serde >>>>>>>> (de)serializers are located under org.apache.kafka.streams.kstream >>>>>>>> package. >>>>>>>> >>>>>>>> What should I do? Should I move my classes under >>>>>>>> org.apache.kafka.streams.kstream package instead? >>>>>>>> >>>>>>>>> On Jul 15, 2019, at 10:45 AM, Development <d...@yeralin.net >>>>>>>>> <mailto:d...@yeralin.net>> wrote: >>>>>>>>> >>>>>>>>> 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 PR. >>>>>>>>> >>>>>>>>> Best, >>>>>>>>> Daniyar Yeralin >>>>>>>>> >>>>>>>>>> On Jul 11, 2019, at 2:44 PM, Matthias J. Sax <matth...@confluent.io >>>>>>>>>> <mailto:matth...@confluent.io>> wrote: >>>>>>>>>> >>>>>>>>>> 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 super user friendly, but all existing code >>>>>>>>>> works this way. Hence, it seems best to stick with the established >>>>>>>>>> pattern. >>>>>>>>>> >>>>>>>>>> We have a similar issue with `TimeWindowedSerde` and >>>>>>>>>> `SessionWindowedSerde`, and I just recently did a PR to improve user >>>>>>>>>> experience that address the exact issue John raised. (cf >>>>>>>>>> https://github.com/apache/kafka/pull/7067 >>>>>>>>>> <https://github.com/apache/kafka/pull/7067>) >>>>>>>>>> >>>>>>>>>> Note, that if a user would instantiate the Serde manually, the user >>>>>>>>>> would also need to call `configure()` to setup the inner serdes. >>>>>>>>>> Kafka >>>>>>>>>> Streams would not setup those automatically and one might most likely >>>>>>>>>> end-up with an NPE. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Coming back the KIP, and the parameter names. `WindowedSerdes` are >>>>>>>>>> similar to `ListSerde` as they wrap another Serde. For >>>>>>>>>> `WindowedSerdes`, >>>>>>>>>> we use the following parameter names: >>>>>>>>>> >>>>>>>>>> - default.windowed.key.serde.inner >>>>>>>>>> - default.windowed.value.serde.inner >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> It might be good to align the naming pattern. I would also suggest to >>>>>>>>>> use `type` instead of `impl`? >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> default.key.list.serde.impl -> default.list.key.serde.type >>>>>>>>>> default.value.list.serde.impl -> default.list.value.serde.type >>>>>>>>>> default.key.list.serde.element -> default.list.key.serde.inner >>>>>>>>>> default.value.list.serde.element -> default.list.value.serde.inner >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -Matthias >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 7/10/19 8:52 AM, Development wrote: >>>>>>>>>>> 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, KafkaStreams cannot use >>>>>>>>>>> it as default serde.” >>>>>>>>>>> >>>>>>>>>>> What do you think about that? I hope I’m not confusing anything. >>>>>>>>>>> >>>>>>>>>>> Best, >>>>>>>>>>> Daniyar Yeralin >>>>>>>>>>> >>>>>>>>>>>> On Jul 9, 2019, at 5:56 PM, John Roesler <j...@confluent.io >>>>>>>>>>>> <mailto:j...@confluent.io>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> 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 <T> Serde<List<T>> ListSerde() in >>>>>>>>>>>>> org.apache.kafka.common.serialization.Serdes class (infers list >>>>>>>>>>>>> implementation and inner serde from config file) >>>>>>>>>>>> >>>>>>>>>>>> It seems like this situation implies my config file is already set >>>>>>>>>>>> up for the list serde, so passing this serde (e.g., in Produced) >>>>>>>>>>>> would have the same effect as not specifying it. >>>>>>>>>>>> >>>>>>>>>>>> I guess that it could be the case that you have the >>>>>>>>>>>> `default.key/value.serde` set to something else, like StringSerde, >>>>>>>>>>>> but you still have the `default.key/value.list.serde.impl/element` >>>>>>>>>>>> set. This seems like it would result in more confusion than >>>>>>>>>>>> convenience, so my gut instinct is maybe we shouldn't introduce >>>>>>>>>>>> the `ListSerde()` variant until people actually request it later >>>>>>>>>>>> on. >>>>>>>>>>>> >>>>>>>>>>>> Thus, we'd just stick with fully config-driven or fully >>>>>>>>>>>> source-code-driven, not half/half. >>>>>>>>>>>> >>>>>>>>>>>> What do you think? >>>>>>>>>>>> >>>>>>>>>>>> Thanks, >>>>>>>>>>>> -John >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Tue, Jul 9, 2019 at 9:58 AM Development <d...@yeralin.net >>>>>>>>>>>> <mailto:d...@yeralin.net> <mailto:d...@yeralin.net >>>>>>>>>>>> <mailto:d...@yeralin.net>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi John, >>>>>>>>>>>>> >>>>>>>>>>>>> I hope everyone had a great long weekend. >>>>>>>>>>>>> >>>>>>>>>>>>> Regarding Java interfaces, I may not understand you correctly, >>>>>>>>>>>>> but I think I already listed them: >>>>>>>>>>>>> >>>>>>>>>>>>> So for Produced, you would use it in the following fashion, for >>>>>>>>>>>>> example: Produced.keySerde(Serdes.ListSerde(ArrayList.class, >>>>>>>>>>>>> Serdes.Integer())) >>>>>>>>>>>>> >>>>>>>>>>>>> I also updated the KIP, and added a section “Serialization >>>>>>>>>>>>> Strategy” where I describe our logic of conditional serialization >>>>>>>>>>>>> based on the type of an inner serde. >>>>>>>>>>>>> >>>>>>>>>>>>> Thank you! >>>>>>>>>>>>> >>>>>>>>>>>>> Best, >>>>>>>>>>>>> Daniyar Yeralin >>>>>>>>>>>>> >>>>>>>>>>>>> On Jun 26, 2019, at 11:44 AM, John Roesler <j...@confluent.io >>>>>>>>>>>>> <mailto:j...@confluent.io> <mailto:j...@confluent.io >>>>>>>>>>>>> <mailto:j...@confluent.io>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> 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 >>>>>>>>>>>>> Serializer >>>>>>>>>>>>> and Deserailizer. I don't think you need to specify the >>>>>>>>>>>>> implementation >>>>>>>>>>>>> logic, since we've already discussed it here. >>>>>>>>>>>>> >>>>>>>>>>>>> If you also want to specify the serialized format of the data >>>>>>>>>>>>> records >>>>>>>>>>>>> in the KIP, it could be useful documentation, as well as letting >>>>>>>>>>>>> us >>>>>>>>>>>>> verify the schema for forward/backward compatibility concerns, >>>>>>>>>>>>> etc. >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> John >>>>>>>>>>>>> >>>>>>>>>>>>> On Wed, Jun 26, 2019 at 10:33 AM Development <d...@yeralin.net >>>>>>>>>>>>> <mailto:d...@yeralin.net> <mailto:d...@yeralin.net >>>>>>>>>>>>> <mailto:d...@yeralin.net>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> 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 >>>>>>>>>>>>> >>>>>>>>>>>>> <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 >>>>>>>>>>>>> >>>>>>>>>>>>> <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:+Add+support+for+List%3CT%3E+serialization+and+deserialization >>>>>>>>>>>>> >>>>>>>>>>>>> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-466:+Add+support+for+List%3CT%3E+serialization+and+deserialization> >>>>>>>>>>>>> >>>>>>>>>>>>> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-466:+Add+support+for+List%3CT%3E+serialization+and+deserialization >>>>>>>>>>>>> >>>>>>>>>>>>> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-466:+Add+support+for+List%3CT%3E+serialization+and+deserialization>>> >>>>>>>>>>>>> Sorry for the delay :) >>>>>>>>>>>>> >>>>>>>>>>>>> Thank You! >>>>>>>>>>>>> >>>>>>>>>>>>> Best, >>>>>>>>>>>>> Daniyar Yeralin >>>>>>>>>>>>> >>>>>>>>>>>>> On Jun 22, 2019, at 12:49 AM, Matthias J. Sax >>>>>>>>>>>>> <matth...@confluent.io <mailto:matth...@confluent.io> >>>>>>>>>>>>> <mailto:matth...@confluent.io <mailto:matth...@confluent.io>>> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> 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 question if I have any). >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> -Matthias >>>>>>>>>>>>> >>>>>>>>>>>>> On 6/21/19 2:05 PM, Development wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Yes, you are right. ByteSerializer is not what I need to have in >>>>>>>>>>>>> a list >>>>>>>>>>>>> of primitives. >>>>>>>>>>>>> >>>>>>>>>>>>> As for the default constructor and configurability, just want to >>>>>>>>>>>>> make >>>>>>>>>>>>> sure. Is this what you have on your mind? >>>>>>>>>>>>> >>>>>>>>>>>>> Best, >>>>>>>>>>>>> Daniyar Yeralin >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Jun 21, 2019, at 2:51 PM, Matthias J. Sax >>>>>>>>>>>>> <matth...@confluent.io <mailto:matth...@confluent.io> >>>>>>>>>>>>> <mailto:matth...@confluent.io <mailto:matth...@confluent.io>> >>>>>>>>>>>>> <mailto:matth...@confluent.io <mailto:matth...@confluent.io> >>>>>>>>>>>>> <mailto:matth...@confluent.io <mailto:matth...@confluent.io>>>> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> 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 serializers: `BytesSerializer` is not primitive >>>>>>>>>>>>> IMHO, >>>>>>>>>>>>> as is it for `byte[]` with variable length -- it's for arrays, >>>>>>>>>>>>> not for >>>>>>>>>>>>> single `byte` (note, that `Bytes` is a Kafka class wrapping >>>>>>>>>>>>> `byte[]`). >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> For tests, we can comment on the PR. No need to do this in the KIP >>>>>>>>>>>>> discussion. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Can you also update the KIP? >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> -Matthias >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On 6/21/19 11:29 AM, Development wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> I made and pushed necessary commits, so we could review the final >>>>>>>>>>>>> version under PR 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>> >>>>>>>>>>>>> >>>>>>>>>>>>> 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 for your help in this KIP :) >>>>>>>>>>>>> >>>>>>>>>>>>> Best, >>>>>>>>>>>>> Daniyar Yeralin >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Jun 21, 2019, at 1:44 PM, John Roesler <j...@confluent.io >>>>>>>>>>>>> <mailto:j...@confluent.io> <mailto:j...@confluent.io >>>>>>>>>>>>> <mailto:j...@confluent.io>> >>>>>>>>>>>>> <mailto:j...@confluent.io <mailto:j...@confluent.io> >>>>>>>>>>>>> <mailto:j...@confluent.io <mailto:j...@confluent.io>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hey Daniyar, >>>>>>>>>>>>> >>>>>>>>>>>>> Looks good to me! Thanks for considering it. >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> -John >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, Jun 21, 2019 at 9:04 AM Development <d...@yeralin.net >>>>>>>>>>>>> <mailto:d...@yeralin.net> <mailto:d...@yeralin.net >>>>>>>>>>>>> <mailto:d...@yeralin.net>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>>> 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 >>>>>>>>>>>>> realized that’s the type is not really needed in ListSerializer, >>>>>>>>>>>>> but >>>>>>>>>>>>> only in ListDeserializer: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> In ListSerializer we will start storing sizes only if serializer >>>>>>>>>>>>> is >>>>>>>>>>>>> not a primitive serializer: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Then, in deserializer, we persist passed list type, so that during >>>>>>>>>>>>> deserialization we could create an instance of it with predefined >>>>>>>>>>>>> listSize for better performance. >>>>>>>>>>>>> We also try to locate a primitiveSize based on passed >>>>>>>>>>>>> deserializer. >>>>>>>>>>>>> If it is not there, then primitiveSize will be null. Which means >>>>>>>>>>>>> that each entry’s size was encoded individually. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> This looks much cleaner and more concise. >>>>>>>>>>>>> >>>>>>>>>>>>> What do you think? >>>>>>>>>>>>> >>>>>>>>>>>>> Best, >>>>>>>>>>>>> Daniyar Yeralin >>>>>>>>>>>>> >>>>>>>>>>>>> On Jun 20, 2019, at 5:45 PM, Matthias J. Sax >>>>>>>>>>>>> <matth...@confluent.io <mailto:matth...@confluent.io> >>>>>>>>>>>>> <mailto:matth...@confluent.io <mailto:matth...@confluent.io>> >>>>>>>>>>>>> <mailto:matth...@confluent.io <mailto:matth...@confluent.io> >>>>>>>>>>>>> <mailto:matth...@confluent.io <mailto:matth...@confluent.io>>> >>>>>>>>>>>>> <mailto:matth...@confluent.io <mailto:matth...@confluent.io> >>>>>>>>>>>>> <mailto:matth...@confluent.io <mailto:matth...@confluent.io>>>> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> 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 types: I don't think we need to encode >>>>>>>>>>>>> the >>>>>>>>>>>>> type size, but users could specify the type on the deserializer >>>>>>>>>>>>> (via a >>>>>>>>>>>>> config again)? >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> About generics: nesting could be arbitrarily deep. Hence, I doubt >>>>>>>>>>>>> we can >>>>>>>>>>>>> support this and a cast will be necessary at some point in the >>>>>>>>>>>>> user >>>>>>>>>>>>> code. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> -Matthias >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On 6/20/19 1:21 PM, John Roesler wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> 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 type is a Map<String,String>? The serde >>>>>>>>>>>>> could >>>>>>>>>>>>> only be used to produce a LinkedList<Map>, thus, we'd still need >>>>>>>>>>>>> an >>>>>>>>>>>>> inner serde, like you have in the KIP (Serde<T> innerSerde). >>>>>>>>>>>>> >>>>>>>>>>>>> Something more like Serde<LinkedList<MyRecord>> = >>>>>>>>>>>>> Serdes.listSerde( >>>>>>>>>>>>> /**list type**/ LinkedList.class, >>>>>>>>>>>>> /**inner serde**/ new MyRecordSerde() >>>>>>>>>>>>> ) >>>>>>>>>>>>> >>>>>>>>>>>>> And in configuration, it's something like: >>>>>>>>>>>>> default.key.serde: org...ListSerde >>>>>>>>>>>>> default.key.list.serde.type: java.util.LinkedList >>>>>>>>>>>>> default.key.list.serde.inner: com.mycompany.MyRecordSerde >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> What do you think? >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> -John >>>>>>>>>>>>> >>>>>>>>>>>>> On Thu, Jun 20, 2019 at 2:46 PM Development <d...@yeralin.net >>>>>>>>>>>>> <mailto:d...@yeralin.net> <mailto:d...@yeralin.net >>>>>>>>>>>>> <mailto:d...@yeralin.net>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> 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 >>>>>>>>>>>>> <https://github.com/FasterXML/jackson-databind/issues/1490> >>>>>>>>>>>>> <https://github.com/FasterXML/jackson-databind/issues/1490 >>>>>>>>>>>>> <https://github.com/FasterXML/jackson-databind/issues/1490>> >>>>>>>>>>>>> <https://github.com/FasterXML/jackson-databind/issues/1490 >>>>>>>>>>>>> <https://github.com/FasterXML/jackson-databind/issues/1490> >>>>>>>>>>>>> <https://github.com/FasterXML/jackson-databind/issues/1490 >>>>>>>>>>>>> <https://github.com/FasterXML/jackson-databind/issues/1490>>> >>>>>>>>>>>>> The only way is to pass an actual class object into the >>>>>>>>>>>>> constructor, >>>>>>>>>>>>> something like: >>>>>>>>>>>>> >>>>>>>>>>>>> It could be an option, but not a pretty one. What do you think of >>>>>>>>>>>>> my >>>>>>>>>>>>> approach to use vanilla java and canonical class name? (As >>>>>>>>>>>>> described >>>>>>>>>>>>> previously) >>>>>>>>>>>>> >>>>>>>>>>>>> Best, >>>>>>>>>>>>> Daniyar Yeralin >>>>>>>>>>>>> >>>>>>>>>>>>> On Jun 20, 2019, at 2:45 PM, Development <d...@yeralin.net >>>>>>>>>>>>> <mailto:d...@yeralin.net> <mailto:d...@yeralin.net >>>>>>>>>>>>> <mailto:d...@yeralin.net>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> 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 <j...@confluent.io >>>>>>>>>>>>> <mailto:j...@confluent.io> <mailto:j...@confluent.io >>>>>>>>>>>>> <mailto:j...@confluent.io>> >>>>>>>>>>>>> <mailto:j...@confluent.io <mailto:j...@confluent.io> >>>>>>>>>>>>> <mailto:j...@confluent.io <mailto:j...@confluent.io>>> >>>>>>>>>>>>> <mailto:j...@confluent.io <mailto:j...@confluent.io> >>>>>>>>>>>>> <mailto:j...@confluent.io <mailto:j...@confluent.io>>> >>>>>>>>>>>>> <mailto:j...@confluent.io <mailto:j...@confluent.io> >>>>>>>>>>>>> <mailto:j...@confluent.io <mailto:j...@confluent.io>> >>>>>>>>>>>>> <mailto:j...@confluent.io <mailto:j...@confluent.io> >>>>>>>>>>>>> <mailto:j...@confluent.io <mailto:j...@confluent.io>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> 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, which in general >>>>>>>>>>>>> require >>>>>>>>>>>>> some kind of schema registry (not necessarily Schema >>>>>>>>>>>>> Registry, just >>>>>>>>>>>>> any registry for schemas) to solve. >>>>>>>>>>>>> >>>>>>>>>>>>> Notice that every serialized record has quite a bit of duplicated >>>>>>>>>>>>> information: the concrete type as well as a byte to indicate >>>>>>>>>>>>> whether >>>>>>>>>>>>> the value type is a fixed size, and, if so, an integer to >>>>>>>>>>>>> indicate the >>>>>>>>>>>>> actual size. These constitute a schema, of sorts, because they >>>>>>>>>>>>> tell us >>>>>>>>>>>>> later how exactly to deserialize the data. Unfortunately, this >>>>>>>>>>>>> information is completely redundant. In all likelihood, the >>>>>>>>>>>>> information will be exactly the same for every record in the >>>>>>>>>>>>> topic. >>>>>>>>>>>>> This problem is essentially the core motivation for serializations >>>>>>>>>>>>> like Avro: to move the schema outside of the serialization >>>>>>>>>>>>> itself, so >>>>>>>>>>>>> that the records won't contain so much redundant information. >>>>>>>>>>>>> >>>>>>>>>>>>> In this light, I'm wondering if it makes sense to go back to >>>>>>>>>>>>> something >>>>>>>>>>>>> like what you had earlier in which you don't support perfectly >>>>>>>>>>>>> preserving the concrete type for _this_ serde, but instead just >>>>>>>>>>>>> support deserializing to _some_ List. Then, you could defer full, >>>>>>>>>>>>> perfect, type preservation to serdes that have an external >>>>>>>>>>>>> system in >>>>>>>>>>>>> which to register their type information. >>>>>>>>>>>>> >>>>>>>>>>>>> There does exist an alternative, if we really do want to >>>>>>>>>>>>> preserve the >>>>>>>>>>>>> concrete type (which does seem kind of nice). You can add a >>>>>>>>>>>>> configuration option specifically for the serde to configure >>>>>>>>>>>>> what the >>>>>>>>>>>>> list type will be, and maybe what the element type is, as well. >>>>>>>>>>>>> >>>>>>>>>>>>> As far as "related work" goes, you might be interested to take >>>>>>>>>>>>> a look >>>>>>>>>>>>> at how Jackson can be configured to deserialize into a specific, >>>>>>>>>>>>> arbitrarily nested, generically parameterized class structure. >>>>>>>>>>>>> Specifically, you might find >>>>>>>>>>>>> https://fasterxml.github.io/jackson-core/javadoc/2.0.0/com/fasterxml/jackson/core/type/TypeReference.html >>>>>>>>>>>>> >>>>>>>>>>>>> <https://fasterxml.github.io/jackson-core/javadoc/2.0.0/com/fasterxml/jackson/core/type/TypeReference.html> >>>>>>>>>>>>> >>>>>>>>>>>>> <https://fasterxml.github.io/jackson-core/javadoc/2.0.0/com/fasterxml/jackson/core/type/TypeReference.html >>>>>>>>>>>>> >>>>>>>>>>>>> <https://fasterxml.github.io/jackson-core/javadoc/2.0.0/com/fasterxml/jackson/core/type/TypeReference.html>> >>>>>>>>>>>>> <https://fasterxml.github.io/jackson-core/javadoc/2.0.0/com/fasterxml/jackson/core/type/TypeReference.html >>>>>>>>>>>>> >>>>>>>>>>>>> <https://fasterxml.github.io/jackson-core/javadoc/2.0.0/com/fasterxml/jackson/core/type/TypeReference.html> >>>>>>>>>>>>> >>>>>>>>>>>>> <https://fasterxml.github.io/jackson-core/javadoc/2.0.0/com/fasterxml/jackson/core/type/TypeReference.html >>>>>>>>>>>>> >>>>>>>>>>>>> <https://fasterxml.github.io/jackson-core/javadoc/2.0.0/com/fasterxml/jackson/core/type/TypeReference.html>>> >>>>>>>>>>>>> interesting. >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> -John >>>>>>>>>>>>> >>>>>>>>>>>>> On Mon, Jun 17, 2019 at 12:38 PM Development <d...@yeralin.net >>>>>>>>>>>>> <mailto:d...@yeralin.net> <mailto:d...@yeralin.net >>>>>>>>>>>>> <mailto:d...@yeralin.net>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net> >>>>>>>>>>>>> <mailto:d...@yeralin.net <mailto:d...@yeralin.net>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> bump >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>> >>> >> >> >
