kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantine Karantasis <konstant...@confluent.io>
Subject Re: [DISCUSS] KIP-440: Extend Connect Converter to support headers
Date Mon, 29 Apr 2019 23:18:50 GMT
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 <rhauch@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 sapiensy@gmail.com <sapiensy@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 <rhauch@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 sapiensy@gmail.com <sapiensy@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 <rhauch@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 <
> > > > > konstantine@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 <rhauch@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 <rhauch@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
<
> > > > > > sapiensy@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
> > > > > > > >>
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message