Re: [VOTE] KIP-145: Expose Record Headers in Kafka Connect

[Randall Hauch]

Thanks for the votes, everyone. This KIP passes with three binding +1 votes
and 4 non-binding +1s (including mine).

Randall

On Tue, Jan 23, 2018 at 3:19 PM, Michael Pearce <michael.pea...@ig.com>
wrote:

> +1 (non-binding)
>
> I personally think the current proposed Converters described in the KIP
> are good, as Randall states it keeps it more in line with the pattern of
> the Converter methods, I think this is a good reason.
>
>
>
> -Original Message-
> From: Jason Gustafson [mailto:ja...@confluent.io]
> Sent: Tuesday, January 23, 2018 8:03 PM
> To: dev@kafka.apache.org
> Subject: Re: [VOTE] KIP-145: Expose Record Headers in Kafka Connect
>
> Hey Randall,
>
> It seemed a bit cleaner to me, but I'm not sure if whether there is any
> advantage to keeping the interfaces decoupled. I'd probably suggest going
> for the simpler API unless you can think of a good reason not to.
>
> -Jason
>
> On Tue, Jan 23, 2018 at 8:47 AM, Randall Hauch <rha...@gmail.com> wrote:
>
> > I mostly just followed the pattern of the Converter methods, which
> > also take the individual components. Really, the only advantage of the
> > current approach is that the HeaderConverter implementations are a bit
> > more decoupled as they are not aware of the Header/Headers API nor the
> > implementation classes, and they don't instantiate the Header instance.
> > Instead, the runtime is entirely responsible for that.
> >
> > However, using the Header interface directly in the method parameters
> > is certainly a bit cleaner from an API perspective. I'm open to this
> > suggestion if you or anyone else prefers it. It'd be a minor change at
> > this point.
> >
> > Randall
> >
> > On Mon, Jan 22, 2018 at 7:30 PM, Jason Gustafson <ja...@confluent.io>
> > wrote:
> >
> > > +1 (binding)
> > >
> > > Just one minor comment. It seems a little surprising that
> > > HeaderConverter does not use the Header interface. I expected
> something like this:
> > >
> > > Header toConnectHeader(String topic, String headerKey, byte[]
> > > value); byte[] fromConnectHeader(String topic, Header header);
> > >
> > > Was there a reason not to do it this way?
> > >
> > > Thanks,
> > > Jason
> > >
> > > On Mon, Jan 22, 2018 at 4:45 PM, Ted Yu <yuzhih...@gmail.com> wrote:
> > >
> > > > +1
> > > >
> > > > On Mon, Jan 22, 2018 at 2:48 PM, Gwen Shapira <g...@confluent.io>
> > wrote:
> > > >
> > > > > +1 (binding)
> > > > >
> > > > > This is going to be HUGE! Thank you Randall.
> > > > >
> > > > > On Mon, Jan 22, 2018 at 1:18 PM Konstantine Karantasis <
> > > > > konstant...@confluent.io> wrote:
> > > > >
> > > > > > Great addition!
> > > > > >
> > > > > > +1 (non-binding)
> > > > > >
> > > > > > Konstantine
> > > > > >
> > > > > > On Sun, Jan 21, 2018 at 7:26 PM, Ewen Cheslack-Postava <
> > > > > e...@confluent.io>
> > > > > > wrote:
> > > > > >
> > > > > > > +1 (binding)
> > > > > > >
> > > > > > > Thanks for the work on this -- not a small upgrade to the
> > > > > > > Connect
> > > > APIs!
> > > > > > >
> > > > > > > -Ewen
> > > > > > >
> > > > > > > On Fri, Jan 19, 2018 at 3:37 PM, Randall Hauch
> > > > > > > <rha...@gmail.com
> > >
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi everyone,
> > > > > > > >
> > > > > > > > I'd like to start the voting on this KIP to add support
> > > > > > > > for
> > > headers
> > > > > in
> > > > > > > > Connect.:
> > > > > > > >
> > > > > > > > *https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > > > 145+-+Expose+Record+Headers+in+Kafka+Connect
> > > > > > > > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > > > 145+-+Expose+Record+Headers+in+Kafka+Connect>*
> > > > > > > >
> > > > > > > > This does add a fair number of interfaces to our pub

RE: [VOTE] KIP-145: Expose Record Headers in Kafka Connect

[Michael Pearce]

+1 (non-binding)

I personally think the current proposed Converters described in the KIP are 
good, as Randall states it keeps it more in line with the pattern of the 
Converter methods, I think this is a good reason.



-Original Message-
From: Jason Gustafson [mailto:ja...@confluent.io]
Sent: Tuesday, January 23, 2018 8:03 PM
To: dev@kafka.apache.org
Subject: Re: [VOTE] KIP-145: Expose Record Headers in Kafka Connect

Hey Randall,

It seemed a bit cleaner to me, but I'm not sure if whether there is any 
advantage to keeping the interfaces decoupled. I'd probably suggest going for 
the simpler API unless you can think of a good reason not to.

-Jason

On Tue, Jan 23, 2018 at 8:47 AM, Randall Hauch <rha...@gmail.com> wrote:

> I mostly just followed the pattern of the Converter methods, which
> also take the individual components. Really, the only advantage of the
> current approach is that the HeaderConverter implementations are a bit
> more decoupled as they are not aware of the Header/Headers API nor the
> implementation classes, and they don't instantiate the Header instance.
> Instead, the runtime is entirely responsible for that.
>
> However, using the Header interface directly in the method parameters
> is certainly a bit cleaner from an API perspective. I'm open to this
> suggestion if you or anyone else prefers it. It'd be a minor change at
> this point.
>
> Randall
>
> On Mon, Jan 22, 2018 at 7:30 PM, Jason Gustafson <ja...@confluent.io>
> wrote:
>
> > +1 (binding)
> >
> > Just one minor comment. It seems a little surprising that
> > HeaderConverter does not use the Header interface. I expected something 
> > like this:
> >
> > Header toConnectHeader(String topic, String headerKey, byte[]
> > value); byte[] fromConnectHeader(String topic, Header header);
> >
> > Was there a reason not to do it this way?
> >
> > Thanks,
> > Jason
> >
> > On Mon, Jan 22, 2018 at 4:45 PM, Ted Yu <yuzhih...@gmail.com> wrote:
> >
> > > +1
> > >
> > > On Mon, Jan 22, 2018 at 2:48 PM, Gwen Shapira <g...@confluent.io>
> wrote:
> > >
> > > > +1 (binding)
> > > >
> > > > This is going to be HUGE! Thank you Randall.
> > > >
> > > > On Mon, Jan 22, 2018 at 1:18 PM Konstantine Karantasis <
> > > > konstant...@confluent.io> wrote:
> > > >
> > > > > Great addition!
> > > > >
> > > > > +1 (non-binding)
> > > > >
> > > > > Konstantine
> > > > >
> > > > > On Sun, Jan 21, 2018 at 7:26 PM, Ewen Cheslack-Postava <
> > > > e...@confluent.io>
> > > > > wrote:
> > > > >
> > > > > > +1 (binding)
> > > > > >
> > > > > > Thanks for the work on this -- not a small upgrade to the
> > > > > > Connect
> > > APIs!
> > > > > >
> > > > > > -Ewen
> > > > > >
> > > > > > On Fri, Jan 19, 2018 at 3:37 PM, Randall Hauch
> > > > > > <rha...@gmail.com
> >
> > > > wrote:
> > > > > >
> > > > > > > Hi everyone,
> > > > > > >
> > > > > > > I'd like to start the voting on this KIP to add support
> > > > > > > for
> > headers
> > > > in
> > > > > > > Connect.:
> > > > > > >
> > > > > > > *https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > > 145+-+Expose+Record+Headers+in+Kafka+Connect
> > > > > > > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > > 145+-+Expose+Record+Headers+in+Kafka+Connect>*
> > > > > > >
> > > > > > > This does add a fair number of interfaces to our public
> > > > > > > API,
> and
> > > > > defines
> > > > > > > some behavioral changes as well.
> > > > > > >
> > > > > > > Thanks! Your feedback is highly appreciated.
> > > > > > >
> > > > > > > Randall
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
The information contained in this email is strictly confidential and for the 
use of the addressee only, unless otherwise indicated. If you are not the 
intended recipient, please do not read, copy, use or disclose to others this 
message or any attachment. Please also notify the sender by replying to this 
email or by telephone (+44(020 7896 0011) and then delete the email and any 
copies of it. Opinions, conclusion (etc) that do not relate to the official 
business of this company shall be understood as neither given nor endorsed by 
it. IG is a trading name of IG Markets Limited (a company registered in England 
and Wales, company number 04008957) and IG Index Limited (a company registered 
in England and Wales, company number 01190902). Registered address at Cannon 
Bridge House, 25 Dowgate Hill, London EC4R 2YA. Both IG Markets Limited 
(register number 195355) and IG Index Limited (register number 114059) are 
authorised and regulated by the Financial Conduct Authority.

Re: [VOTE] KIP-145: Expose Record Headers in Kafka Connect

[Jason Gustafson]

Hey Randall,

It seemed a bit cleaner to me, but I'm not sure if whether there is any
advantage to keeping the interfaces decoupled. I'd probably suggest going
for the simpler API unless you can think of a good reason not to.

-Jason

On Tue, Jan 23, 2018 at 8:47 AM, Randall Hauch  wrote:

> I mostly just followed the pattern of the Converter methods, which also
> take the individual components. Really, the only advantage of the current
> approach is that the HeaderConverter implementations are a bit more
> decoupled as they are not aware of the Header/Headers API nor the
> implementation classes, and they don't instantiate the Header instance.
> Instead, the runtime is entirely responsible for that.
>
> However, using the Header interface directly in the method parameters is
> certainly a bit cleaner from an API perspective. I'm open to this
> suggestion if you or anyone else prefers it. It'd be a minor change at this
> point.
>
> Randall
>
> On Mon, Jan 22, 2018 at 7:30 PM, Jason Gustafson 
> wrote:
>
> > +1 (binding)
> >
> > Just one minor comment. It seems a little surprising that HeaderConverter
> > does not use the Header interface. I expected something like this:
> >
> > Header toConnectHeader(String topic, String headerKey, byte[] value);
> > byte[] fromConnectHeader(String topic, Header header);
> >
> > Was there a reason not to do it this way?
> >
> > Thanks,
> > Jason
> >
> > On Mon, Jan 22, 2018 at 4:45 PM, Ted Yu  wrote:
> >
> > > +1
> > >
> > > On Mon, Jan 22, 2018 at 2:48 PM, Gwen Shapira 
> wrote:
> > >
> > > > +1 (binding)
> > > >
> > > > This is going to be HUGE! Thank you Randall.
> > > >
> > > > On Mon, Jan 22, 2018 at 1:18 PM Konstantine Karantasis <
> > > > konstant...@confluent.io> wrote:
> > > >
> > > > > Great addition!
> > > > >
> > > > > +1 (non-binding)
> > > > >
> > > > > Konstantine
> > > > >
> > > > > On Sun, Jan 21, 2018 at 7:26 PM, Ewen Cheslack-Postava <
> > > > e...@confluent.io>
> > > > > wrote:
> > > > >
> > > > > > +1 (binding)
> > > > > >
> > > > > > Thanks for the work on this -- not a small upgrade to the Connect
> > > APIs!
> > > > > >
> > > > > > -Ewen
> > > > > >
> > > > > > On Fri, Jan 19, 2018 at 3:37 PM, Randall Hauch  >
> > > > wrote:
> > > > > >
> > > > > > > Hi everyone,
> > > > > > >
> > > > > > > I'd like to start the voting on this KIP to add support for
> > headers
> > > > in
> > > > > > > Connect.:
> > > > > > >
> > > > > > > *https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > > 145+-+Expose+Record+Headers+in+Kafka+Connect
> > > > > > >  > > > > > > 145+-+Expose+Record+Headers+in+Kafka+Connect>*
> > > > > > >
> > > > > > > This does add a fair number of interfaces to our public API,
> and
> > > > > defines
> > > > > > > some behavioral changes as well.
> > > > > > >
> > > > > > > Thanks! Your feedback is highly appreciated.
> > > > > > >
> > > > > > > Randall
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-145: Expose Record Headers in Kafka Connect

[Randall Hauch]

I mostly just followed the pattern of the Converter methods, which also
take the individual components. Really, the only advantage of the current
approach is that the HeaderConverter implementations are a bit more
decoupled as they are not aware of the Header/Headers API nor the
implementation classes, and they don't instantiate the Header instance.
Instead, the runtime is entirely responsible for that.

However, using the Header interface directly in the method parameters is
certainly a bit cleaner from an API perspective. I'm open to this
suggestion if you or anyone else prefers it. It'd be a minor change at this
point.

Randall

On Mon, Jan 22, 2018 at 7:30 PM, Jason Gustafson  wrote:

> +1 (binding)
>
> Just one minor comment. It seems a little surprising that HeaderConverter
> does not use the Header interface. I expected something like this:
>
> Header toConnectHeader(String topic, String headerKey, byte[] value);
> byte[] fromConnectHeader(String topic, Header header);
>
> Was there a reason not to do it this way?
>
> Thanks,
> Jason
>
> On Mon, Jan 22, 2018 at 4:45 PM, Ted Yu  wrote:
>
> > +1
> >
> > On Mon, Jan 22, 2018 at 2:48 PM, Gwen Shapira  wrote:
> >
> > > +1 (binding)
> > >
> > > This is going to be HUGE! Thank you Randall.
> > >
> > > On Mon, Jan 22, 2018 at 1:18 PM Konstantine Karantasis <
> > > konstant...@confluent.io> wrote:
> > >
> > > > Great addition!
> > > >
> > > > +1 (non-binding)
> > > >
> > > > Konstantine
> > > >
> > > > On Sun, Jan 21, 2018 at 7:26 PM, Ewen Cheslack-Postava <
> > > e...@confluent.io>
> > > > wrote:
> > > >
> > > > > +1 (binding)
> > > > >
> > > > > Thanks for the work on this -- not a small upgrade to the Connect
> > APIs!
> > > > >
> > > > > -Ewen
> > > > >
> > > > > On Fri, Jan 19, 2018 at 3:37 PM, Randall Hauch 
> > > wrote:
> > > > >
> > > > > > Hi everyone,
> > > > > >
> > > > > > I'd like to start the voting on this KIP to add support for
> headers
> > > in
> > > > > > Connect.:
> > > > > >
> > > > > > *https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > 145+-+Expose+Record+Headers+in+Kafka+Connect
> > > > > >  > > > > > 145+-+Expose+Record+Headers+in+Kafka+Connect>*
> > > > > >
> > > > > > This does add a fair number of interfaces to our public API, and
> > > > defines
> > > > > > some behavioral changes as well.
> > > > > >
> > > > > > Thanks! Your feedback is highly appreciated.
> > > > > >
> > > > > > Randall
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-145: Expose Record Headers in Kafka Connect

[Jason Gustafson]

+1 (binding)

Just one minor comment. It seems a little surprising that HeaderConverter
does not use the Header interface. I expected something like this:

Header toConnectHeader(String topic, String headerKey, byte[] value);
byte[] fromConnectHeader(String topic, Header header);

Was there a reason not to do it this way?

Thanks,
Jason

On Mon, Jan 22, 2018 at 4:45 PM, Ted Yu  wrote:

> +1
>
> On Mon, Jan 22, 2018 at 2:48 PM, Gwen Shapira  wrote:
>
> > +1 (binding)
> >
> > This is going to be HUGE! Thank you Randall.
> >
> > On Mon, Jan 22, 2018 at 1:18 PM Konstantine Karantasis <
> > konstant...@confluent.io> wrote:
> >
> > > Great addition!
> > >
> > > +1 (non-binding)
> > >
> > > Konstantine
> > >
> > > On Sun, Jan 21, 2018 at 7:26 PM, Ewen Cheslack-Postava <
> > e...@confluent.io>
> > > wrote:
> > >
> > > > +1 (binding)
> > > >
> > > > Thanks for the work on this -- not a small upgrade to the Connect
> APIs!
> > > >
> > > > -Ewen
> > > >
> > > > On Fri, Jan 19, 2018 at 3:37 PM, Randall Hauch 
> > wrote:
> > > >
> > > > > Hi everyone,
> > > > >
> > > > > I'd like to start the voting on this KIP to add support for headers
> > in
> > > > > Connect.:
> > > > >
> > > > > *https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > 145+-+Expose+Record+Headers+in+Kafka+Connect
> > > > >  > > > > 145+-+Expose+Record+Headers+in+Kafka+Connect>*
> > > > >
> > > > > This does add a fair number of interfaces to our public API, and
> > > defines
> > > > > some behavioral changes as well.
> > > > >
> > > > > Thanks! Your feedback is highly appreciated.
> > > > >
> > > > > Randall
> > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-145: Expose Record Headers in Kafka Connect

[Ted Yu]

+1

On Mon, Jan 22, 2018 at 2:48 PM, Gwen Shapira  wrote:

> +1 (binding)
>
> This is going to be HUGE! Thank you Randall.
>
> On Mon, Jan 22, 2018 at 1:18 PM Konstantine Karantasis <
> konstant...@confluent.io> wrote:
>
> > Great addition!
> >
> > +1 (non-binding)
> >
> > Konstantine
> >
> > On Sun, Jan 21, 2018 at 7:26 PM, Ewen Cheslack-Postava <
> e...@confluent.io>
> > wrote:
> >
> > > +1 (binding)
> > >
> > > Thanks for the work on this -- not a small upgrade to the Connect APIs!
> > >
> > > -Ewen
> > >
> > > On Fri, Jan 19, 2018 at 3:37 PM, Randall Hauch 
> wrote:
> > >
> > > > Hi everyone,
> > > >
> > > > I'd like to start the voting on this KIP to add support for headers
> in
> > > > Connect.:
> > > >
> > > > *https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > 145+-+Expose+Record+Headers+in+Kafka+Connect
> > > >  > > > 145+-+Expose+Record+Headers+in+Kafka+Connect>*
> > > >
> > > > This does add a fair number of interfaces to our public API, and
> > defines
> > > > some behavioral changes as well.
> > > >
> > > > Thanks! Your feedback is highly appreciated.
> > > >
> > > > Randall
> > > >
> > >
> >
>

Re: [VOTE] KIP-145: Expose Record Headers in Kafka Connect

[Gwen Shapira]

+1 (binding)

This is going to be HUGE! Thank you Randall.

On Mon, Jan 22, 2018 at 1:18 PM Konstantine Karantasis <
konstant...@confluent.io> wrote:

> Great addition!
>
> +1 (non-binding)
>
> Konstantine
>
> On Sun, Jan 21, 2018 at 7:26 PM, Ewen Cheslack-Postava 
> wrote:
>
> > +1 (binding)
> >
> > Thanks for the work on this -- not a small upgrade to the Connect APIs!
> >
> > -Ewen
> >
> > On Fri, Jan 19, 2018 at 3:37 PM, Randall Hauch  wrote:
> >
> > > Hi everyone,
> > >
> > > I'd like to start the voting on this KIP to add support for headers in
> > > Connect.:
> > >
> > > *https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 145+-+Expose+Record+Headers+in+Kafka+Connect
> > >  > > 145+-+Expose+Record+Headers+in+Kafka+Connect>*
> > >
> > > This does add a fair number of interfaces to our public API, and
> defines
> > > some behavioral changes as well.
> > >
> > > Thanks! Your feedback is highly appreciated.
> > >
> > > Randall
> > >
> >
>

Re: [VOTE] KIP-145: Expose Record Headers in Kafka Connect

[Konstantine Karantasis]

Great addition!

+1 (non-binding)

Konstantine

On Sun, Jan 21, 2018 at 7:26 PM, Ewen Cheslack-Postava 
wrote:

> +1 (binding)
>
> Thanks for the work on this -- not a small upgrade to the Connect APIs!
>
> -Ewen
>
> On Fri, Jan 19, 2018 at 3:37 PM, Randall Hauch  wrote:
>
> > Hi everyone,
> >
> > I'd like to start the voting on this KIP to add support for headers in
> > Connect.:
> >
> > *https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 145+-+Expose+Record+Headers+in+Kafka+Connect
> >  > 145+-+Expose+Record+Headers+in+Kafka+Connect>*
> >
> > This does add a fair number of interfaces to our public API, and defines
> > some behavioral changes as well.
> >
> > Thanks! Your feedback is highly appreciated.
> >
> > Randall
> >
>

Re: [VOTE] KIP-145: Expose Record Headers in Kafka Connect

[Ewen Cheslack-Postava]

+1 (binding)

Thanks for the work on this -- not a small upgrade to the Connect APIs!

-Ewen

On Fri, Jan 19, 2018 at 3:37 PM, Randall Hauch  wrote:

> Hi everyone,
>
> I'd like to start the voting on this KIP to add support for headers in
> Connect.:
>
> *https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 145+-+Expose+Record+Headers+in+Kafka+Connect
>  145+-+Expose+Record+Headers+in+Kafka+Connect>*
>
> This does add a fair number of interfaces to our public API, and defines
> some behavioral changes as well.
>
> Thanks! Your feedback is highly appreciated.
>
> Randall
>

[VOTE] KIP-145: Expose Record Headers in Kafka Connect

[Randall Hauch]

Hi everyone,

I'd like to start the voting on this KIP to add support for headers in
Connect.:

*https://cwiki.apache.org/confluence/display/KAFKA/KIP-145+-+Expose+Record+Headers+in+Kafka+Connect
*

This does add a fair number of interfaces to our public API, and defines
some behavioral changes as well.

Thanks! Your feedback is highly appreciated.

Randall