+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.
