Re: [DISCUSS] KIP-440: Extend Connect Converter to support headers

[sapiensy]

Hi Konstantine, Randall,

As you can see in the updated Converter interface, it always operates on 
`org.apache.kafka.common.header.Headers`. 

WorkerSinkTask simply uses Kafka message headers and passes them to the 
`toConnectData` method.

WorkerSourceTask leverages header converter to extract RecordHeaders, which 
implements Headers interface. Then RecordHeaders are passed to the 
`fromConnectData` method.

So header converter is used as a way to get headers when converting data from 
internal Connect format to Kafka messages (cause there is no other way to get 
the headers in this case).

I can add this to the KIP if it's helpful.

Randall, what is the alternative approach you're referring to?

On 2019/04/22 18:09:24, Randall Hauch  wrote: 
> Konstantine raises a good point. Which `Headers` is being referenced in the
> API? The Connect `org.apache.kafka.connect.header.Headers` would correspond
> to what was already deserialized by the `HeaderConverter` or what will yet
> be serialized by the `HeaderConverter`. Alternatively, the common `
> org.apache.kafka.common.header.Headers` would correspond to the raw header
> pairs from the underlying Kafka record.
> 
> So, we probably want to be a bit more specific, and also mention why. And
> we probably want to mention the other approach in the rejected alternatives.
> 
> Best regards,
> 
> Randall
> 
> On Mon, Apr 22, 2019 at 11:59 AM Konstantine Karantasis <
> konstant...@confluent.io> wrote:
> 
> > Thanks for the KIP Yaroslav!
> >
> > Apologies for the late comment. However, after reading the KIP it's still
> > not very clear to me what happens to the existing
> > HeaderConverter interface and what's the expectation for existing code
> > implementing this interface out there.
> >
> > Looking at the PR I see that the existing code is leaving the existing
> > connect headers conversion unaffected. I'd expect by reading the KIP to
> > understand what's the interplay of the current proposal with the existing
> > implementation of KIP-145 that introduced headers in Connect.
> >
> > Thanks,
> > Konstantine
> >
> > On Mon, Apr 22, 2019 at 9:07 AM Randall Hauch  wrote:
> >
> > > Thanks for updating the KIP. It looks good to me, and since there haven't
> > > been any other issue mentioned in this month-long thread, it's probably
> > > fine to start a vote.
> > >
> > > Best regards,
> > >
> > > Randall
> > >
> > > On Tue, Apr 2, 2019 at 3:12 PM Randall Hauch  wrote:
> > >
> > > > Thanks for the submission, Yaroslav -- and for building on the
> > suggestion
> > > > of Jeremy C in https://issues.apache.org/jira/browse/KAFKA-7273. This
> > is
> > > > a nice and simple approach that is backward compatible.
> > > >
> > > > The KIP looks good so far, but I do have two specific suggestions to
> > make
> > > > things just a bit more explicit. First, both the "Public API" and
> > > "Proposed
> > > > Changes" sections could be more explicit that the methods in the
> > proposal
> > > > are being added; as it's currently written a reader must infer that.
> > > > Second, the "Proposed Changes" section needs to more clearly specify
> > that
> > > > the WorkerSourceTask will now use the new fromConnectData method with
> > the
> > > > headers instead of the existing method, and that the WorkerSinkTask
> > will
> > > > now use the toConnectData method with the headers instead of the
> > existing
> > > > method.
> > > >
> > > > Best regards,
> > > >
> > > > Randall
> > > >
> > > >
> > > > On Mon, Mar 11, 2019 at 11:01 PM Yaroslav Tkachenko <
> > sapie...@gmail.com>
> > > > wrote:
> > > >
> > > >> Hello,
> > > >>
> > > >> I'd like to propose a KIP that extends Kafka Connect Converter
> > > interface:
> > > >>
> > > >>
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-440%3A+Extend+Connect+Converter+to+support+headers
> > > >>
> > > >> Thanks for considering!
> > > >>
> > > >> --
> > > >> Yaroslav Tkachenko
> > > >> sap1ens.com
> > > >>
> > > >
> > >
> >
> 

Re: [DISCUSS] KIP-440: Extend Connect Converter to support headers

[sapiensy]

Hi Randall, Konstantine,

I've updated the KIP to reflect the details we discussed here. Let me know if 
it looks good and we can go to the voting phase.

Thanks!

On 2019/04/22 21:07:31, Randall Hauch  wrote: 
> I think it would be helpful to clarify this in the KIP, just so that
> readers are aware that the headers will be the raw header bytes that are
> the same as what is in the Kafka record.
> 
> The alternative I was referring to is exposing the Connect `Headers`
> interface, which is different.
> 
> On Mon, Apr 22, 2019 at 1:45 PM sapie...@gmail.com 
> wrote:
> 
> > Hi Konstantine, Randall,
> >
> > As you can see in the updated Converter interface, it always operates on
> > `org.apache.kafka.common.header.Headers`.
> >
> > WorkerSinkTask simply uses Kafka message headers and passes them to the
> > `toConnectData` method.
> >
> > WorkerSourceTask leverages header converter to extract RecordHeaders,
> > which implements Headers interface. Then RecordHeaders are passed to the
> > `fromConnectData` method.
> >
> > So header converter is used as a way to get headers when converting data
> > from internal Connect format to Kafka messages (cause there is no other way
> > to get the headers in this case).
> >
> > I can add this to the KIP if it's helpful.
> >
> > Randall, what is the alternative approach you're referring to?
> >
> > On 2019/04/22 18:09:24, Randall Hauch  wrote:
> > > Konstantine raises a good point. Which `Headers` is being referenced in
> > the
> > > API? The Connect `org.apache.kafka.connect.header.Headers` would
> > correspond
> > > to what was already deserialized by the `HeaderConverter` or what will
> > yet
> > > be serialized by the `HeaderConverter`. Alternatively, the common `
> > > org.apache.kafka.common.header.Headers` would correspond to the raw
> > header
> > > pairs from the underlying Kafka record.
> > >
> > > So, we probably want to be a bit more specific, and also mention why. And
> > > we probably want to mention the other approach in the rejected
> > alternatives.
> > >
> > > Best regards,
> > >
> > > Randall
> > >
> > > On Mon, Apr 22, 2019 at 11:59 AM Konstantine Karantasis <
> > > konstant...@confluent.io> wrote:
> > >
> > > > Thanks for the KIP Yaroslav!
> > > >
> > > > Apologies for the late comment. However, after reading the KIP it's
> > still
> > > > not very clear to me what happens to the existing
> > > > HeaderConverter interface and what's the expectation for existing code
> > > > implementing this interface out there.
> > > >
> > > > Looking at the PR I see that the existing code is leaving the existing
> > > > connect headers conversion unaffected. I'd expect by reading the KIP to
> > > > understand what's the interplay of the current proposal with the
> > existing
> > > > implementation of KIP-145 that introduced headers in Connect.
> > > >
> > > > Thanks,
> > > > Konstantine
> > > >
> > > > On Mon, Apr 22, 2019 at 9:07 AM Randall Hauch 
> > wrote:
> > > >
> > > > > Thanks for updating the KIP. It looks good to me, and since there
> > haven't
> > > > > been any other issue mentioned in this month-long thread, it's
> > probably
> > > > > fine to start a vote.
> > > > >
> > > > > Best regards,
> > > > >
> > > > > Randall
> > > > >
> > > > > On Tue, Apr 2, 2019 at 3:12 PM Randall Hauch 
> > wrote:
> > > > >
> > > > > > Thanks for the submission, Yaroslav -- and for building on the
> > > > suggestion
> > > > > > of Jeremy C in https://issues.apache.org/jira/browse/KAFKA-7273.
> > This
> > > > is
> > > > > > a nice and simple approach that is backward compatible.
> > > > > >
> > > > > > The KIP looks good so far, but I do have two specific suggestions
> > to
> > > > make
> > > > > > things just a bit more explicit. First, both the "Public API" and
> > > > > "Proposed
> > > > > > Changes" sections could be more explicit that the methods in the
> > > > proposal
> > > > > > are being added; as it's currently written a reader must infer
> > that.
> > > > > > Second, the "Proposed Changes" section needs to more clearly
> > specify
> > > > that
> > > > > > the WorkerSourceTask will now use the new fromConnectData method
> > with
> > > > the
> > > > > > headers instead of the existing method, and that the WorkerSinkTask
> > > > will
> > > > > > now use the toConnectData method with the headers instead of the
> > > > existing
> > > > > > method.
> > > > > >
> > > > > > Best regards,
> > > > > >
> > > > > > Randall
> > > > > >
> > > > > >
> > > > > > On Mon, Mar 11, 2019 at 11:01 PM Yaroslav Tkachenko <
> > > > sapie...@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > >> Hello,
> > > > > >>
> > > > > >> I'd like to propose a KIP that extends Kafka Connect Converter
> > > > > interface:
> > > > > >>
> > > > > >>
> > > > >
> > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-440%3A+Extend+Connect+Converter+to+support+headers
> > > > > >>
> > > > > >> Thanks for considering!
> > > > > >>
> > > > > >> --
> > > > > >> Yaroslav Tkachenko
> > > > > >> sap1ens.c

Re: [DISCUSS] KIP-440: Extend Connect Converter to support headers

[sapiensy]

Hi Randall,

Sorry, I was away. Just started a vote a few minutes ago.

On 2019/05/06 17:46:23, Randall Hauch  wrote: 
> Have we started a vote on this? I don't see the separate "[VOTE]" thread.
> 
> On Mon, Apr 29, 2019 at 6:19 PM Konstantine Karantasis <
> konstant...@confluent.io> wrote:
> 
> > Thanks Yaroslav, this KIP LGTM now too!
> >
> > To give some context regarding my previous comment: headers in Connect
> > would probably have followed a similar approach if default methods in
> > interfaces could be used at the time. But we could not have assumed java 8
> > or later yet in the AK version that Connect headers were added, so, I
> > believe, that led to two different converter interfaces.
> >
> > Thanks for the nicely written KIP!
> > Konstantine
> >
> >
> >
> > On Mon, Apr 29, 2019 at 3:39 PM Randall Hauch  wrote:
> >
> > > Thanks for the update. Yes, IMO this KIP is ready for a vote.
> > >
> > > On Fri, Apr 26, 2019 at 12:15 AM sapie...@gmail.com 
> > > wrote:
> > >
> > > > Hi Randall, Konstantine,
> > > >
> > > > I've updated the KIP to reflect the details we discussed here. Let me
> > > know
> > > > if it looks good and we can go to the voting phase.
> > > >
> > > > Thanks!
> > > >
> > > > On 2019/04/22 21:07:31, Randall Hauch  wrote:
> > > > > I think it would be helpful to clarify this in the KIP, just so that
> > > > > readers are aware that the headers will be the raw header bytes that
> > > are
> > > > > the same as what is in the Kafka record.
> > > > >
> > > > > The alternative I was referring to is exposing the Connect `Headers`
> > > > > interface, which is different.
> > > > >
> > > > > On Mon, Apr 22, 2019 at 1:45 PM sapie...@gmail.com <
> > sapie...@gmail.com
> > > >
> > > > > wrote:
> > > > >
> > > > > > Hi Konstantine, Randall,
> > > > > >
> > > > > > As you can see in the updated Converter interface, it always
> > operates
> > > > on
> > > > > > `org.apache.kafka.common.header.Headers`.
> > > > > >
> > > > > > WorkerSinkTask simply uses Kafka message headers and passes them to
> > > the
> > > > > > `toConnectData` method.
> > > > > >
> > > > > > WorkerSourceTask leverages header converter to extract
> > RecordHeaders,
> > > > > > which implements Headers interface. Then RecordHeaders are passed
> > to
> > > > the
> > > > > > `fromConnectData` method.
> > > > > >
> > > > > > So header converter is used as a way to get headers when converting
> > > > data
> > > > > > from internal Connect format to Kafka messages (cause there is no
> > > > other way
> > > > > > to get the headers in this case).
> > > > > >
> > > > > > I can add this to the KIP if it's helpful.
> > > > > >
> > > > > > Randall, what is the alternative approach you're referring to?
> > > > > >
> > > > > > On 2019/04/22 18:09:24, Randall Hauch  wrote:
> > > > > > > Konstantine raises a good point. Which `Headers` is being
> > > referenced
> > > > in
> > > > > > the
> > > > > > > API? The Connect `org.apache.kafka.connect.header.Headers` would
> > > > > > correspond
> > > > > > > to what was already deserialized by the `HeaderConverter` or what
> > > > will
> > > > > > yet
> > > > > > > be serialized by the `HeaderConverter`. Alternatively, the
> > common `
> > > > > > > org.apache.kafka.common.header.Headers` would correspond to the
> > raw
> > > > > > header
> > > > > > > pairs from the underlying Kafka record.
> > > > > > >
> > > > > > > So, we probably want to be a bit more specific, and also mention
> > > > why. And
> > > > > > > we probably want to mention the other approach in the rejected
> > > > > > alternatives.
> > > > > > >
> > > > > > > Best regards,
> > > > > > >
> > > > > > > Randall
> > > > > > >
> > > > > > > On Mon, Apr 22, 2019 at 11:59 AM Konstantine Karantasis <
> > > > > > > konstant...@confluent.io> wrote:
> > > > > > >
> > > > > > > > Thanks for the KIP Yaroslav!
> > > > > > > >
> > > > > > > > Apologies for the late comment. However, after reading the KIP
> > > it's
> > > > > > still
> > > > > > > > not very clear to me what happens to the existing
> > > > > > > > HeaderConverter interface and what's the expectation for
> > existing
> > > > code
> > > > > > > > implementing this interface out there.
> > > > > > > >
> > > > > > > > Looking at the PR I see that the existing code is leaving the
> > > > existing
> > > > > > > > connect headers conversion unaffected. I'd expect by reading
> > the
> > > > KIP to
> > > > > > > > understand what's the interplay of the current proposal with
> > the
> > > > > > existing
> > > > > > > > implementation of KIP-145 that introduced headers in Connect.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Konstantine
> > > > > > > >
> > > > > > > > On Mon, Apr 22, 2019 at 9:07 AM Randall Hauch <
> > rha...@gmail.com>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > Thanks for updating the KIP. It looks good to me, and since
> > > there
> > > > > > haven't
> > > > > > > > > been any other issue mentioned in this month-long thread,
> > it's
> > > > > > prob

Re: [VOTE] KIP-440: Extend Connect Converter to support headers

[sapiensy]

Hey everyone, just bumping this thread again. We need one more vote from the 
committers. Thanks! :)

On 2019/05/19 14:31:15, Kamal Chandraprakash  
wrote: 
> +1 (non-binding). Thanks for the KIP!
> 
> On Sun, May 19, 2019 at 6:36 PM Dongjin Lee  wrote:
> 
> > +1 (non-binding).
> >
> > Binding: +2 (Randall, Gwen)
> > Non-binding: +2 (Andrew, Dongjin)
> >
> > We need one more +1 from the committers. Is there anyone else?
> >
> > Thanks,
> > Dongjin
> >
> > On Fri, May 10, 2019 at 12:16 AM Andrew Schofield <
> > andrew_schofi...@live.com>
> > wrote:
> >
> > > +1 (non-binding).
> > >
> > > Looks good.
> > >
> > > On 09/05/2019, 15:55, "Gwen Shapira"  wrote:
> > >
> > > +1 (binding)
> > > Thank you!
> > >
> > > On Mon, May 6, 2019, 11:25 PM Yaroslav Tkachenko  > >
> > > wrote:
> > >
> > > > Hi everyone,
> > > >
> > > > I'd like to start a vote for KIP-440: Extend Connect Converter to
> > > support
> > > > headers (
> > > >
> > > >
> > >
> > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FKAFKA%2FKIP-440%253A%2BExtend%2BConnect%2BConverter%2Bto%2Bsupport%2Bheaders&data=02%7C01%7C%7C82864a9a5f3049e8ca1d08d6d48e5147%7C84df9e7fe9f640afb435%7C1%7C0%7C636930105039335723&sdata=FyQT5pfTwtT2NTMStgSl6zRjiaWFVJqG3%2B4vt5nRYmI%3D&reserved=0
> > > > )
> > > >
> > > > Discussion:
> > > >
> > > >
> > >
> > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.apache.org%2Fthread.html%2F1fc1e3d2cddd8311d3db7c98f0d09a1a137ca4b20d1f3c8ab203a855%40%253Cdev.kafka.apache.org%253E&data=02%7C01%7C%7C82864a9a5f3049e8ca1d08d6d48e5147%7C84df9e7fe9f640afb435%7C1%7C0%7C636930105039335723&sdata=B2a4Mx9ScWaO3HEEw0LoIRX0ajETAcwUmDxt5Ir5FIs%3D&reserved=0
> > > >
> > > > Thanks!
> > > >
> > >
> > > --
> > > *Dongjin Lee*
> > >
> > > *A hitchhiker in the mathematical world.*
> > > *github:  github.com/dongjinleekr
> > > linkedin:
> > kr.linkedin.com/in/dongjinleekr
> > > speakerdeck:
> > speakerdeck.com/dongjin
> > > *
> > >
> >
> 

Re: [VOTE] KIP-440: Extend Connect Converter to support headers

[sapiensy]

Thanks everyone! Closing the vote with +7 (+3 binding). 

On 2019/08/21 13:41:34, Bill Bejeck  wrote: 
> Thanks for the KIP! This looks like a valuable addition.
> 
> +1(binding)
> 
> -Bill
> 
> On Mon, Aug 5, 2019 at 6:15 PM Ryanne Dolan  wrote:
> 
> > +1, non-binding
> >
> > Ryanne
> >
> > On Mon, Aug 5, 2019 at 3:38 PM Randall Hauch  wrote:
> >
> > > If my math is right, we have 3 non-binding +1 votes and 2 binding +1
> > votes.
> > >
> > > This is a simple but really beneficial KIP for Connect. Can we get
> > another
> > > review and vote by a committer? Thanks!
> > >
> > > Randall
> > >
> > > On Fri, May 31, 2019 at 3:37 PM sapie...@gmail.com 
> > > wrote:
> > >
> > > > Hey everyone, just bumping this thread again. We need one more vote
> > from
> > > > the committers. Thanks! :)
> > > >
> > > > On 2019/05/19 14:31:15, Kamal Chandraprakash <
> > > > kamal.chandraprak...@gmail.com> wrote:
> > > > > +1 (non-binding). Thanks for the KIP!
> > > > >
> > > > > On Sun, May 19, 2019 at 6:36 PM Dongjin Lee 
> > > wrote:
> > > > >
> > > > > > +1 (non-binding).
> > > > > >
> > > > > > Binding: +2 (Randall, Gwen)
> > > > > > Non-binding: +2 (Andrew, Dongjin)
> > > > > >
> > > > > > We need one more +1 from the committers. Is there anyone else?
> > > > > >
> > > > > > Thanks,
> > > > > > Dongjin
> > > > > >
> > > > > > On Fri, May 10, 2019 at 12:16 AM Andrew Schofield <
> > > > > > andrew_schofi...@live.com>
> > > > > > wrote:
> > > > > >
> > > > > > > +1 (non-binding).
> > > > > > >
> > > > > > > Looks good.
> > > > > > >
> > > > > > > On 09/05/2019, 15:55, "Gwen Shapira"  wrote:
> > > > > > >
> > > > > > > +1 (binding)
> > > > > > > Thank you!
> > > > > > >
> > > > > > > On Mon, May 6, 2019, 11:25 PM Yaroslav Tkachenko <
> > > > sapie...@gmail.com
> > > > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi everyone,
> > > > > > > >
> > > > > > > > I'd like to start a vote for KIP-440: Extend Connect
> > > Converter
> > > > to
> > > > > > > support
> > > > > > > > headers (
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FKAFKA%2FKIP-440%253A%2BExtend%2BConnect%2BConverter%2Bto%2Bsupport%2Bheaders&data=02%7C01%7C%7C82864a9a5f3049e8ca1d08d6d48e5147%7C84df9e7fe9f640afb435%7C1%7C0%7C636930105039335723&sdata=FyQT5pfTwtT2NTMStgSl6zRjiaWFVJqG3%2B4vt5nRYmI%3D&reserved=0
> > > > > > > > )
> > > > > > > >
> > > > > > > > Discussion:
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.apache.org%2Fthread.html%2F1fc1e3d2cddd8311d3db7c98f0d09a1a137ca4b20d1f3c8ab203a855%40%253Cdev.kafka.apache.org%253E&data=02%7C01%7C%7C82864a9a5f3049e8ca1d08d6d48e5147%7C84df9e7fe9f640afb435%7C1%7C0%7C636930105039335723&sdata=B2a4Mx9ScWaO3HEEw0LoIRX0ajETAcwUmDxt5Ir5FIs%3D&reserved=0
> > > > > > > >
> > > > > > > > Thanks!
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > *Dongjin Lee*
> > > > > > >
> > > > > > > *A hitchhiker in the mathematical world.*
> > > > > > > *github:  github.com/dongjinleekr
> > > > > > > linkedin:
> > > > > > kr.linkedin.com/in/dongjinleekr
> > > > > > > speakerdeck:
> > > > > > speakerdeck.com/dongjin
> > > > > > > *
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>