You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by "379377944@qq.com" <37...@qq.com> on 2020/07/29 02:49:07 UTC
Re: Re: [Discuss] KIP-581: Value of optional null field which has default value
Hi Chris,
Thanks for your good suggestion, the KIP document and draft PR has been updated, please review again.
And I found due to my misoperation, the mail thread has been broken, no idea how to fix it.
Thanks
Cheng Pan
From: Christopher Egerton
Date: 2020-05-04 10:53
To: dev
Subject: Re: [Discuss] KIP-581: Value of optional null field which has default value
Hi Cheng,
I think refactoring that method should be fine (if maybe a little painful);
the method itself is private and all places that it's invoked directly are
either package-private or non-static, so it shouldn't affect any of the
public methods of the JSON converter to change "convertToConnect" to be
non-static. Even if it did, the only parts of the JSON converter that are
public API (and therefore officially subject to concerns about
compatibility) are the methods it implements that satisfy the "Converter"
and "HeaderConverter" interfaces.
Would you mind explicitly specifying in the KIP that the new property will
be added for the JSON converter only, and that it will affect both
serialization and deserialization?
Cheers,
Chris
On Tue, Apr 28, 2020 at 10:52 AM 379377944 <37...@qq.com> wrote:
> Hi Chris,
>
>
> Thanks for your reminder, the original implement is deprecated, I just
> update the JIRA with the new
> PR link: https://github.com/apache/kafka/pull/8575
>
>
> As question 2), I agree with you that we should consider both
> serialization and deserialization, and as you said, I only implement the
> serialization now. This is because the original serde implement is not
> symmetrical, the convertToConnect is a static method and can’t access the
> field in JsonConverter
> instance, maybe I should do some refactoring to implement the
> deserialization.
>
>
> Thanks,
> Cheng Pan
> Original Message
> Sender: Christopher Egerton<ch...@confluent.io>
> Recipient: dev<de...@kafka.apache.org>
> Date: Wednesday, Apr 15, 2020 02:28
> Subject: Re: [Discuss] KIP-581: Value of optional null field which has
> default value
>
>
> 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 <37...@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
Re: [Discuss] KIP-581: Value of optional null field which has default value
Posted by Mickael Maison <mi...@gmail.com>.
Hi Cheng Pan,
Are you still interested in following up with this KIP? This is a
useful feature so it would be nice to get it in Kafka.
Thanks,
Mickael
On Wed, Aug 17, 2022 at 1:39 PM Mickael Maison <mi...@gmail.com> wrote:
>
> Hi Cheng Pan,
>
> Thanks for the updates. I have a few more questions:
>
> 1) Since this KIP is targeting JsonConverter, I think we should add
> this new config to JsonConverterConfig instead of ConverterConfig.
>
> 2) What about renaming the new field to something like
> "use.null.for.optional.fields"?
>
> Thanks,
> Mickael
>
> On Thu, May 5, 2022 at 3:19 PM Cheng Pan <ch...@apache.org> wrote:
> >
> > Update PR links
> >
> > [1] https://github.com/apache/kafka/pull/12126
> >
> > On 2022/05/05 13:16:59 Cheng Pan wrote:
> > > Hi Mickael,
> > >
> > > Thanks for ping me.
> > >
> > > I updated the KIP-581 description to narrow the change scope to address this specific issue, and raised a WIP PR[1] to implement it.
> > >
> > > Please let me know if you have any concerns.
> > >
> > > [1] https://github.com/apache/kafka/pull/12126
> > >
> > > Thanks,
> > > Cheng Pan
> > >
> > > On 2022/05/02 12:57:25 Mickael Maison wrote:
> > > > Hi Cheng Pan,
> > > >
> > > > Thanks for raising this KIP, this would be a useful improvement!
> > > >
> > > > You never started a VOTE thread for this KIP. Are you interested in
> > > > finishing up this work?
> > > > If so, based on the discussion, I think you can open a VOTE thread as
> > > > described in https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals#KafkaImprovementProposals-Process
> > > > If not, it's not a problem, someone else can volunteer to pick it up.
> > > >
> > > > Please let us know.
> > > >
> > > > Thanks,
> > > > Mickael
> > > >
> > > > On Sat, Aug 8, 2020 at 11:05 AM Ruslan Gibaiev <r....@gmail.com> wrote:
> > > > >
> > > > > Hello guys.
> > > > > Proposed PR seems to be fixing the issue in a backward-compatible way.
> > > > > Let's please move forward with it. Would be great to see it included into next Kafka release
> > > > > Thank you
> > > > >
> > > > > On 2020/07/29 02:49:07, "379377944@qq.com" <37...@qq.com> wrote:
> > > > > > Hi Chris,
> > > > > >
> > > > > > Thanks for your good suggestion, the KIP document and draft PR has been updated, please review again.
> > > > > >
> > > > > > And I found due to my misoperation, the mail thread has been broken, no idea how to fix it.
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Thanks
> > > > > > Cheng Pan
> > > > > >
> > > > > > From: Christopher Egerton
> > > > > > Date: 2020-05-04 10:53
> > > > > > To: dev
> > > > > > Subject: Re: [Discuss] KIP-581: Value of optional null field which has default value
> > > > > > Hi Cheng,
> > > > > >
> > > > > > I think refactoring that method should be fine (if maybe a little painful);
> > > > > > the method itself is private and all places that it's invoked directly are
> > > > > > either package-private or non-static, so it shouldn't affect any of the
> > > > > > public methods of the JSON converter to change "convertToConnect" to be
> > > > > > non-static. Even if it did, the only parts of the JSON converter that are
> > > > > > public API (and therefore officially subject to concerns about
> > > > > > compatibility) are the methods it implements that satisfy the "Converter"
> > > > > > and "HeaderConverter" interfaces.
> > > > > >
> > > > > > Would you mind explicitly specifying in the KIP that the new property will
> > > > > > be added for the JSON converter only, and that it will affect both
> > > > > > serialization and deserialization?
> > > > > >
> > > > > > Cheers,
> > > > > >
> > > > > > Chris
> > > > > >
> > > > > > On Tue, Apr 28, 2020 at 10:52 AM 379377944 <37...@qq.com> wrote:
> > > > > >
> > > > > > > Hi Chris,
> > > > > > >
> > > > > > >
> > > > > > > Thanks for your reminder, the original implement is deprecated, I just
> > > > > > > update the JIRA with the new
> > > > > > > PR link: https://github.com/apache/kafka/pull/8575
> > > > > > >
> > > > > > >
> > > > > > > As question 2), I agree with you that we should consider both
> > > > > > > serialization and deserialization, and as you said, I only implement the
> > > > > > > serialization now. This is because the original serde implement is not
> > > > > > > symmetrical, the convertToConnect is a static method and can’t access the
> > > > > > > field in JsonConverter
> > > > > > > instance, maybe I should do some refactoring to implement the
> > > > > > > deserialization.
> > > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Cheng Pan
> > > > > > > Original Message
> > > > > > > Sender: Christopher Egerton<ch...@confluent.io>
> > > > > > > Recipient: dev<de...@kafka.apache.org>
> > > > > > > Date: Wednesday, Apr 15, 2020 02:28
> > > > > > > Subject: Re: [Discuss] KIP-581: Value of optional null field which has
> > > > > > > default value
> > > > > > >
> > > > > > >
> > > > > > > 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 <37...@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
> > > > > >
> > > >
> > >
Re: [Discuss] KIP-581: Value of optional null field which has default value
Posted by Mickael Maison <mi...@gmail.com>.
Hi Cheng Pan,
Thanks for the updates. I have a few more questions:
1) Since this KIP is targeting JsonConverter, I think we should add
this new config to JsonConverterConfig instead of ConverterConfig.
2) What about renaming the new field to something like
"use.null.for.optional.fields"?
Thanks,
Mickael
On Thu, May 5, 2022 at 3:19 PM Cheng Pan <ch...@apache.org> wrote:
>
> Update PR links
>
> [1] https://github.com/apache/kafka/pull/12126
>
> On 2022/05/05 13:16:59 Cheng Pan wrote:
> > Hi Mickael,
> >
> > Thanks for ping me.
> >
> > I updated the KIP-581 description to narrow the change scope to address this specific issue, and raised a WIP PR[1] to implement it.
> >
> > Please let me know if you have any concerns.
> >
> > [1] https://github.com/apache/kafka/pull/12126
> >
> > Thanks,
> > Cheng Pan
> >
> > On 2022/05/02 12:57:25 Mickael Maison wrote:
> > > Hi Cheng Pan,
> > >
> > > Thanks for raising this KIP, this would be a useful improvement!
> > >
> > > You never started a VOTE thread for this KIP. Are you interested in
> > > finishing up this work?
> > > If so, based on the discussion, I think you can open a VOTE thread as
> > > described in https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals#KafkaImprovementProposals-Process
> > > If not, it's not a problem, someone else can volunteer to pick it up.
> > >
> > > Please let us know.
> > >
> > > Thanks,
> > > Mickael
> > >
> > > On Sat, Aug 8, 2020 at 11:05 AM Ruslan Gibaiev <r....@gmail.com> wrote:
> > > >
> > > > Hello guys.
> > > > Proposed PR seems to be fixing the issue in a backward-compatible way.
> > > > Let's please move forward with it. Would be great to see it included into next Kafka release
> > > > Thank you
> > > >
> > > > On 2020/07/29 02:49:07, "379377944@qq.com" <37...@qq.com> wrote:
> > > > > Hi Chris,
> > > > >
> > > > > Thanks for your good suggestion, the KIP document and draft PR has been updated, please review again.
> > > > >
> > > > > And I found due to my misoperation, the mail thread has been broken, no idea how to fix it.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Thanks
> > > > > Cheng Pan
> > > > >
> > > > > From: Christopher Egerton
> > > > > Date: 2020-05-04 10:53
> > > > > To: dev
> > > > > Subject: Re: [Discuss] KIP-581: Value of optional null field which has default value
> > > > > Hi Cheng,
> > > > >
> > > > > I think refactoring that method should be fine (if maybe a little painful);
> > > > > the method itself is private and all places that it's invoked directly are
> > > > > either package-private or non-static, so it shouldn't affect any of the
> > > > > public methods of the JSON converter to change "convertToConnect" to be
> > > > > non-static. Even if it did, the only parts of the JSON converter that are
> > > > > public API (and therefore officially subject to concerns about
> > > > > compatibility) are the methods it implements that satisfy the "Converter"
> > > > > and "HeaderConverter" interfaces.
> > > > >
> > > > > Would you mind explicitly specifying in the KIP that the new property will
> > > > > be added for the JSON converter only, and that it will affect both
> > > > > serialization and deserialization?
> > > > >
> > > > > Cheers,
> > > > >
> > > > > Chris
> > > > >
> > > > > On Tue, Apr 28, 2020 at 10:52 AM 379377944 <37...@qq.com> wrote:
> > > > >
> > > > > > Hi Chris,
> > > > > >
> > > > > >
> > > > > > Thanks for your reminder, the original implement is deprecated, I just
> > > > > > update the JIRA with the new
> > > > > > PR link: https://github.com/apache/kafka/pull/8575
> > > > > >
> > > > > >
> > > > > > As question 2), I agree with you that we should consider both
> > > > > > serialization and deserialization, and as you said, I only implement the
> > > > > > serialization now. This is because the original serde implement is not
> > > > > > symmetrical, the convertToConnect is a static method and can’t access the
> > > > > > field in JsonConverter
> > > > > > instance, maybe I should do some refactoring to implement the
> > > > > > deserialization.
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Cheng Pan
> > > > > > Original Message
> > > > > > Sender: Christopher Egerton<ch...@confluent.io>
> > > > > > Recipient: dev<de...@kafka.apache.org>
> > > > > > Date: Wednesday, Apr 15, 2020 02:28
> > > > > > Subject: Re: [Discuss] KIP-581: Value of optional null field which has
> > > > > > default value
> > > > > >
> > > > > >
> > > > > > 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 <37...@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
> > > > >
> > >
> >
Re: [Discuss] KIP-581: Value of optional null field which has default value
Posted by Cheng Pan <ch...@apache.org>.
Update PR links
[1] https://github.com/apache/kafka/pull/12126
On 2022/05/05 13:16:59 Cheng Pan wrote:
> Hi Mickael,
>
> Thanks for ping me.
>
> I updated the KIP-581 description to narrow the change scope to address this specific issue, and raised a WIP PR[1] to implement it.
>
> Please let me know if you have any concerns.
>
> [1] https://github.com/apache/kafka/pull/12126
>
> Thanks,
> Cheng Pan
>
> On 2022/05/02 12:57:25 Mickael Maison wrote:
> > Hi Cheng Pan,
> >
> > Thanks for raising this KIP, this would be a useful improvement!
> >
> > You never started a VOTE thread for this KIP. Are you interested in
> > finishing up this work?
> > If so, based on the discussion, I think you can open a VOTE thread as
> > described in https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals#KafkaImprovementProposals-Process
> > If not, it's not a problem, someone else can volunteer to pick it up.
> >
> > Please let us know.
> >
> > Thanks,
> > Mickael
> >
> > On Sat, Aug 8, 2020 at 11:05 AM Ruslan Gibaiev <r....@gmail.com> wrote:
> > >
> > > Hello guys.
> > > Proposed PR seems to be fixing the issue in a backward-compatible way.
> > > Let's please move forward with it. Would be great to see it included into next Kafka release
> > > Thank you
> > >
> > > On 2020/07/29 02:49:07, "379377944@qq.com" <37...@qq.com> wrote:
> > > > Hi Chris,
> > > >
> > > > Thanks for your good suggestion, the KIP document and draft PR has been updated, please review again.
> > > >
> > > > And I found due to my misoperation, the mail thread has been broken, no idea how to fix it.
> > > >
> > > >
> > > >
> > > >
> > > > Thanks
> > > > Cheng Pan
> > > >
> > > > From: Christopher Egerton
> > > > Date: 2020-05-04 10:53
> > > > To: dev
> > > > Subject: Re: [Discuss] KIP-581: Value of optional null field which has default value
> > > > Hi Cheng,
> > > >
> > > > I think refactoring that method should be fine (if maybe a little painful);
> > > > the method itself is private and all places that it's invoked directly are
> > > > either package-private or non-static, so it shouldn't affect any of the
> > > > public methods of the JSON converter to change "convertToConnect" to be
> > > > non-static. Even if it did, the only parts of the JSON converter that are
> > > > public API (and therefore officially subject to concerns about
> > > > compatibility) are the methods it implements that satisfy the "Converter"
> > > > and "HeaderConverter" interfaces.
> > > >
> > > > Would you mind explicitly specifying in the KIP that the new property will
> > > > be added for the JSON converter only, and that it will affect both
> > > > serialization and deserialization?
> > > >
> > > > Cheers,
> > > >
> > > > Chris
> > > >
> > > > On Tue, Apr 28, 2020 at 10:52 AM 379377944 <37...@qq.com> wrote:
> > > >
> > > > > Hi Chris,
> > > > >
> > > > >
> > > > > Thanks for your reminder, the original implement is deprecated, I just
> > > > > update the JIRA with the new
> > > > > PR link: https://github.com/apache/kafka/pull/8575
> > > > >
> > > > >
> > > > > As question 2), I agree with you that we should consider both
> > > > > serialization and deserialization, and as you said, I only implement the
> > > > > serialization now. This is because the original serde implement is not
> > > > > symmetrical, the convertToConnect is a static method and can’t access the
> > > > > field in JsonConverter
> > > > > instance, maybe I should do some refactoring to implement the
> > > > > deserialization.
> > > > >
> > > > >
> > > > > Thanks,
> > > > > Cheng Pan
> > > > > Original Message
> > > > > Sender: Christopher Egerton<ch...@confluent.io>
> > > > > Recipient: dev<de...@kafka.apache.org>
> > > > > Date: Wednesday, Apr 15, 2020 02:28
> > > > > Subject: Re: [Discuss] KIP-581: Value of optional null field which has
> > > > > default value
> > > > >
> > > > >
> > > > > 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 <37...@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
> > > >
> >
>
Re: [Discuss] KIP-581: Value of optional null field which has default value
Posted by Cheng Pan <ch...@apache.org>.
Hi Mickael,
Thanks for ping me.
I updated the KIP-581 description to narrow the change scope to address this specific issue, and raised a WIP PR[1] to implement it.
Please let me know if you have any concerns.
[1] https://github.com/apache/kafka/pull/8575
Thanks,
Cheng Pan
On 2022/05/02 12:57:25 Mickael Maison wrote:
> Hi Cheng Pan,
>
> Thanks for raising this KIP, this would be a useful improvement!
>
> You never started a VOTE thread for this KIP. Are you interested in
> finishing up this work?
> If so, based on the discussion, I think you can open a VOTE thread as
> described in https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals#KafkaImprovementProposals-Process
> If not, it's not a problem, someone else can volunteer to pick it up.
>
> Please let us know.
>
> Thanks,
> Mickael
>
> On Sat, Aug 8, 2020 at 11:05 AM Ruslan Gibaiev <r....@gmail.com> wrote:
> >
> > Hello guys.
> > Proposed PR seems to be fixing the issue in a backward-compatible way.
> > Let's please move forward with it. Would be great to see it included into next Kafka release
> > Thank you
> >
> > On 2020/07/29 02:49:07, "379377944@qq.com" <37...@qq.com> wrote:
> > > Hi Chris,
> > >
> > > Thanks for your good suggestion, the KIP document and draft PR has been updated, please review again.
> > >
> > > And I found due to my misoperation, the mail thread has been broken, no idea how to fix it.
> > >
> > >
> > >
> > >
> > > Thanks
> > > Cheng Pan
> > >
> > > From: Christopher Egerton
> > > Date: 2020-05-04 10:53
> > > To: dev
> > > Subject: Re: [Discuss] KIP-581: Value of optional null field which has default value
> > > Hi Cheng,
> > >
> > > I think refactoring that method should be fine (if maybe a little painful);
> > > the method itself is private and all places that it's invoked directly are
> > > either package-private or non-static, so it shouldn't affect any of the
> > > public methods of the JSON converter to change "convertToConnect" to be
> > > non-static. Even if it did, the only parts of the JSON converter that are
> > > public API (and therefore officially subject to concerns about
> > > compatibility) are the methods it implements that satisfy the "Converter"
> > > and "HeaderConverter" interfaces.
> > >
> > > Would you mind explicitly specifying in the KIP that the new property will
> > > be added for the JSON converter only, and that it will affect both
> > > serialization and deserialization?
> > >
> > > Cheers,
> > >
> > > Chris
> > >
> > > On Tue, Apr 28, 2020 at 10:52 AM 379377944 <37...@qq.com> wrote:
> > >
> > > > Hi Chris,
> > > >
> > > >
> > > > Thanks for your reminder, the original implement is deprecated, I just
> > > > update the JIRA with the new
> > > > PR link: https://github.com/apache/kafka/pull/8575
> > > >
> > > >
> > > > As question 2), I agree with you that we should consider both
> > > > serialization and deserialization, and as you said, I only implement the
> > > > serialization now. This is because the original serde implement is not
> > > > symmetrical, the convertToConnect is a static method and can’t access the
> > > > field in JsonConverter
> > > > instance, maybe I should do some refactoring to implement the
> > > > deserialization.
> > > >
> > > >
> > > > Thanks,
> > > > Cheng Pan
> > > > Original Message
> > > > Sender: Christopher Egerton<ch...@confluent.io>
> > > > Recipient: dev<de...@kafka.apache.org>
> > > > Date: Wednesday, Apr 15, 2020 02:28
> > > > Subject: Re: [Discuss] KIP-581: Value of optional null field which has
> > > > default value
> > > >
> > > >
> > > > 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 <37...@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
> > >
>
Re: Re: [Discuss] KIP-581: Value of optional null field which has default value
Posted by Mickael Maison <mi...@apache.org>.
Hi Cheng Pan,
Thanks for raising this KIP, this would be a useful improvement!
You never started a VOTE thread for this KIP. Are you interested in
finishing up this work?
If so, based on the discussion, I think you can open a VOTE thread as
described in https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals#KafkaImprovementProposals-Process
If not, it's not a problem, someone else can volunteer to pick it up.
Please let us know.
Thanks,
Mickael
On Sat, Aug 8, 2020 at 11:05 AM Ruslan Gibaiev <r....@gmail.com> wrote:
>
> Hello guys.
> Proposed PR seems to be fixing the issue in a backward-compatible way.
> Let's please move forward with it. Would be great to see it included into next Kafka release
> Thank you
>
> On 2020/07/29 02:49:07, "379377944@qq.com" <37...@qq.com> wrote:
> > Hi Chris,
> >
> > Thanks for your good suggestion, the KIP document and draft PR has been updated, please review again.
> >
> > And I found due to my misoperation, the mail thread has been broken, no idea how to fix it.
> >
> >
> >
> >
> > Thanks
> > Cheng Pan
> >
> > From: Christopher Egerton
> > Date: 2020-05-04 10:53
> > To: dev
> > Subject: Re: [Discuss] KIP-581: Value of optional null field which has default value
> > Hi Cheng,
> >
> > I think refactoring that method should be fine (if maybe a little painful);
> > the method itself is private and all places that it's invoked directly are
> > either package-private or non-static, so it shouldn't affect any of the
> > public methods of the JSON converter to change "convertToConnect" to be
> > non-static. Even if it did, the only parts of the JSON converter that are
> > public API (and therefore officially subject to concerns about
> > compatibility) are the methods it implements that satisfy the "Converter"
> > and "HeaderConverter" interfaces.
> >
> > Would you mind explicitly specifying in the KIP that the new property will
> > be added for the JSON converter only, and that it will affect both
> > serialization and deserialization?
> >
> > Cheers,
> >
> > Chris
> >
> > On Tue, Apr 28, 2020 at 10:52 AM 379377944 <37...@qq.com> wrote:
> >
> > > Hi Chris,
> > >
> > >
> > > Thanks for your reminder, the original implement is deprecated, I just
> > > update the JIRA with the new
> > > PR link: https://github.com/apache/kafka/pull/8575
> > >
> > >
> > > As question 2), I agree with you that we should consider both
> > > serialization and deserialization, and as you said, I only implement the
> > > serialization now. This is because the original serde implement is not
> > > symmetrical, the convertToConnect is a static method and can’t access the
> > > field in JsonConverter
> > > instance, maybe I should do some refactoring to implement the
> > > deserialization.
> > >
> > >
> > > Thanks,
> > > Cheng Pan
> > > Original Message
> > > Sender: Christopher Egerton<ch...@confluent.io>
> > > Recipient: dev<de...@kafka.apache.org>
> > > Date: Wednesday, Apr 15, 2020 02:28
> > > Subject: Re: [Discuss] KIP-581: Value of optional null field which has
> > > default value
> > >
> > >
> > > 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 <37...@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
> >
Re: Re: [Discuss] KIP-581: Value of optional null field which has default value
Posted by Ruslan Gibaiev <r....@gmail.com>.
Hello guys.
Proposed PR seems to be fixing the issue in a backward-compatible way.
Let's please move forward with it. Would be great to see it included into next Kafka release
Thank you
On 2020/07/29 02:49:07, "379377944@qq.com" <37...@qq.com> wrote:
> Hi Chris,
>
> Thanks for your good suggestion, the KIP document and draft PR has been updated, please review again.
>
> And I found due to my misoperation, the mail thread has been broken, no idea how to fix it.
>
>
>
>
> Thanks
> Cheng Pan
>
> From: Christopher Egerton
> Date: 2020-05-04 10:53
> To: dev
> Subject: Re: [Discuss] KIP-581: Value of optional null field which has default value
> Hi Cheng,
>
> I think refactoring that method should be fine (if maybe a little painful);
> the method itself is private and all places that it's invoked directly are
> either package-private or non-static, so it shouldn't affect any of the
> public methods of the JSON converter to change "convertToConnect" to be
> non-static. Even if it did, the only parts of the JSON converter that are
> public API (and therefore officially subject to concerns about
> compatibility) are the methods it implements that satisfy the "Converter"
> and "HeaderConverter" interfaces.
>
> Would you mind explicitly specifying in the KIP that the new property will
> be added for the JSON converter only, and that it will affect both
> serialization and deserialization?
>
> Cheers,
>
> Chris
>
> On Tue, Apr 28, 2020 at 10:52 AM 379377944 <37...@qq.com> wrote:
>
> > Hi Chris,
> >
> >
> > Thanks for your reminder, the original implement is deprecated, I just
> > update the JIRA with the new
> > PR link: https://github.com/apache/kafka/pull/8575
> >
> >
> > As question 2), I agree with you that we should consider both
> > serialization and deserialization, and as you said, I only implement the
> > serialization now. This is because the original serde implement is not
> > symmetrical, the convertToConnect is a static method and can’t access the
> > field in JsonConverter
> > instance, maybe I should do some refactoring to implement the
> > deserialization.
> >
> >
> > Thanks,
> > Cheng Pan
> > Original Message
> > Sender: Christopher Egerton<ch...@confluent.io>
> > Recipient: dev<de...@kafka.apache.org>
> > Date: Wednesday, Apr 15, 2020 02:28
> > Subject: Re: [Discuss] KIP-581: Value of optional null field which has
> > default value
> >
> >
> > 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 <37...@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
>