Thanks Tom and Chris for your feedback! I agree one configuration is enough. I've renamed it to 'replace.null.with.default'. Since you both seemed happy with the KIP overall, I'll start a vote later today.
Thanks, Mickael On Thu, Mar 16, 2023 at 7:08 PM Chris Egerton <chr...@aiven.io.invalid> wrote: > > Hi Mickael, > > Tom got this part perfect so I'm just going to copy+paste: Thanks to Cheng > for the KIP and to you for picking it up. > > I'm wondering why we need separate properties for serialization vs. > deserialization? If the converter is being used by the Kafka Connect > runtime, a given instance of it will only ever be responsible for one or > the other (in other words, sink connectors' converters will only ever be > used for deserialization and source connectors' converters will only ever > be used for serialization). It seems like a single "use.optional.null" (or > "map.null.to.default") property would be simpler and easier to use, but I > might be missing something about why we'd want to add this kind of > granularity. > > I'm slightly in favor of the alternative name that Tom has proposed. I > think highlighting that this property deals with how to handle default > values is important, possibly more important than the fact that it deals > with null field values. I'm a little hesitant to use "map" since that may > be harder to remember and at first glance it might seem like it deals with > the map schema type. Maybe "replace.null.with.default.value"? (For the > record, I don't consider any of this worthy of blocking the KIP, so don't > feel the need to appease me on this front before starting a vote.) > > Cheers, > > Chris > > On Thu, Mar 16, 2023 at 11:38 AM Tom Bentley <tbent...@redhat.com> wrote: > > > Hi Mickael, > > > > Thanks to Cheng for the KIP and to you for picking it up. > > > > My only comment (feel free to ignore) is about the names of the configs. > > Personally I don't think I'd correctly guess what > > "serialize.use.optional.null" meant. Something like > > "serialize.map.null.to.default" is much clearer to me, for the cost of one > > extra token. > > > > Otherwise LGTM. > > > > Thanks, > > > > Tom > > > > On Wed, 8 Mar 2023 at 15:55, Mickael Maison <mickael.mai...@gmail.com> > > wrote: > > > > > Hi, > > > > > > This KIP has been staled for a long time. Since it would be a useful > > > feature, I pinged Cheng about a month ago asking if he was planning to > > > work on it again. I've not received a reply, so I've allowed myself to > > > update the KIP (hopefully preserving the initial requirements) and > > > would like to restart a discussion. > > > > > > The DISCUSS thread was split in two, you can find the other part in > > > https://lists.apache.org/thread/dc56k17zptzvbyc7gtscovzgzwf6yn9p > > > > > > Let me know if you have any feedback. > > > > > > Thanks, > > > Mickael > > > > > > On Tue, Apr 14, 2020 at 8:28 PM Christopher Egerton <chr...@confluent.io > > > > > > wrote: > > > > > > > > Hi Cheng, > > > > > > > > Thanks for the KIP! I really appreciate the care that was taken to > > ensure > > > > backwards compatibility for existing users, and the minimal changes to > > > > public interface that are suggested to address this. > > > > > > > > I have two quick requests for clarification: > > > > > > > > 1) Where is the proposed "accept.optional.null" property going to > > apply? > > > > It's hinted that it'll take effect on the JSON converter but not > > actually > > > > called out anywhere. > > > > > > > > 2) Assuming this takes effect on the JSON converter, is the intent to > > > alter > > > > the semantics for both serialization and deserialization? The code > > > snippet > > > > from the JSON converter that's included in the KIP comes from the > > > > "convertToJson" method, which is used for serialization. However, based > > > on > > > > > > > > > https://github.com/apache/kafka/blob/ea47a885b1fe47dfb87c1dc86db1b0e7eb8a273c/connect/json/src/main/java/org/apache/kafka/connect/json/JsonConverter.java#L712-L713 > > > > it > > > > looks like the converter also inserts the default value for > > > > optional-but-null data during deserialization. > > > > > > > > Thanks again for the KIP! > > > > > > > > Cheers, > > > > > > > > Chris > > > > > > > > On Wed, Mar 18, 2020 at 12:00 AM Cheng Pan <379377...@qq.com> wrote: > > > > > > > > > Hi all, > > > > > > > > > > I'd like to use this thread to discuss KIP-581: Value of optional > > null > > > > > field which has default value, please see detail at: > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-581%3A+Value+of+optional+null+field+which+has+default+value > > > > > > > > > > > > > > > There are some previous discussion at: > > > > > https://github.com/apache/kafka/pull/7112 > > > > > > > > > > > > > > > I'm a beginner for apache project, please let me know if I did any > > > thing > > > > > wrong. > > > > > > > > > > > > > > > Best regards, > > > > > Cheng Pan > > > > > > > >
