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