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