LGTM On Fri, Mar 17, 2023, 08:26 Luke Chen <show...@gmail.com> wrote:
> Thanks Mickael and Cheng! > This KIP LGTM! > > Thanks. > Luke > > On Fri, Mar 17, 2023 at 7:27 PM Mickael Maison <mickael.mai...@gmail.com> > wrote: > > > 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 > > > > > > > > > > > > > > > > >
