Hi Randall, Sorry, I was away. Just started a vote a few minutes ago.
On 2019/05/06 17:46:23, Randall Hauch <rha...@gmail.com> 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 <rha...@gmail.com> 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 <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 <rha...@gmail.com> 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 <rha...@gmail.com> 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 > > > > > > probably > > > > > > > > > fine to start a vote. > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > > > > > > > > Randall > > > > > > > > > > > > > > > > > > On Tue, Apr 2, 2019 at 3:12 PM Randall Hauch < > > rha...@gmail.com > > > > > > > > > > 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 > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
