kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jason Gustafson <ja...@confluent.io>
Subject Re: [DISCUSS] KIP-82 - Add Record Headers
Date Wed, 22 Feb 2017 22:09:02 GMT
The current producer interceptor API is this:

ProducerRecord<K, V> onSend(ProducerRecord<K, V> record);

So adding a header means creating a new ProducerRecord with a new header
added to the current headers and returning it. Would that not work?

-Jason

On Wed, Feb 22, 2017 at 1:45 PM, Michael Pearce <Michael.Pearce@ig.com>
wrote:

> So how would you have this work if not mutable where interceptors would
> add headers?
>
> Sent using OWA for iPhone
> ________________________________________
> From: Jason Gustafson <jason@confluent.io>
> Sent: Wednesday, February 22, 2017 8:42:27 PM
> To: dev@kafka.apache.org
> Subject: Re: [DISCUSS] KIP-82 - Add Record Headers
>
> I think the point on the mutability of Headers is worth discussing a little
> more. As far as I can tell, once the ProducerRecord (or ConsumerRecord) is
> constructed, there should be no need to further change the headers. Is that
> correct? If so, then why not enforce that that is the case through the API?
> One problem with mutability it that it constrains the implementation of
> Headers. For example, if we were backing with a byte slice, would we recopy
> the bytes if a header is added or would we maintain a satellite collection
> of added records. Seems not great either way. If we really think mutability
> is needed, perhaps we could add a method to headers to convert it to a
> mutable type (e.g. a Headers.toMap)?
>
> I'm also with Ismael about exposing Headers.get(). I thought it might make
> sense to have a method like this instead:
>
> Iterable<Header> findMatching(Pattern pattern);
>
> This makes the (potential) need to scan the headers clear in the API. I'd
> also be fine exposing no getter at all. In general, Ï think it's good to
> start with a minimalistic API and work from there since it's always easier
> to add methods than remove them.
>
> -Jason
>
> On Wed, Feb 22, 2017 at 9:16 AM, Michael Pearce <Michael.Pearce@ig.com>
> wrote:
>
> >
> > Hi Ismael
> >
> > On point 1,
> >
> > Sure makes sense will update shortly.
> >
> > On point 2,
> >
> > Setter/getter typical to properties/headers api’s traditionally are map
> > styled interfaces and what I believe is most expected styled thus the
> Key,
> > Value setter.
> > Also it would mean rather than an interface, we would be making our
> > internal header impl object we have for the array, exposed. E.g. if we
> had
> > a Map really this would be Map.Entry interface, this is the same reasons
> on
> > the map interface I cannot actually make the underlying Node object
> that’s
> > the implementation for HashMap, so that internals can be changed.
> >
> >
> > On point 3,
> >
> > I think it people do expect it to be performant, thus why originally
> > concern I raised with using an array for to me is an early memory
> > optimisation. I think the user experience of properties/headers is on a
> > get/set model. This is why its important we have encapsulated logic that
> > then allows us to change this to a map, if this becomes and issue, and
> the
> > memory overhead of hashmap is less so.
> >
> >
> >
> >
> > On 22/02/2017, 15:56, "ismaelj@gmail.com on behalf of Ismael Juma" <
> > ismaelj@gmail.com on behalf of ismael@juma.me.uk> wrote:
> >
> >     Hi all,
> >
> >     Great to see the progress that has been achieved on this one. :) A
> few
> >     comments regarding the APIs (I'm still reviewing the message format
> >     changes):
> >
> >     1. Nit: `getHeaders` in `ProducerRecord` and `ConsumerRecord` should
> be
> >     named `headers` (we avoid the `get` prefix in Kafka)
> >
> >     2. The `Headers` class is mutable (there's an `add` method). Does it
> > need
> >     to be? If so, it would be good to explain why. Related to that, we
> > should
> >     also explain the thinking around thread-safety. If we keep the `add`
> >     method, it may make sense for it to take a `Header` (that way we can
> > add
> >     things to `Header` without changing the interface).
> >
> >     3. Do we need the `Headers.get()` method? People usually assume that
> > `get`
> >     would be efficient, but depending on the implementation (the current
> >     proposal states that an array would be used), it may not be. If we
> > expect
> >     the number of headers to be small, it doesn't matter though.
> >
> >     Ismael
> >
> >     On Tue, Feb 21, 2017 at 6:38 PM, Michael Pearce <
> Michael.Pearce@ig.com
> > >
> >     wrote:
> >
> >     > Hi Jason,
> >     >
> >     > Have converted the interface/api bullets into interface code
> > snippets.
> >     >
> >     > Agreed implementation won’t take too long. We have early versions
> > already.
> >     > Maybe a week before you think about merging I would assume it would
> > be more
> >     > stabilised? I was thinking then we could fork from your confluent
> > branch,
> >     > making and then holding KIP-82 changes in a patch file, that we can
> > then
> >     > re-fork from apache once KIP98 final is merged, and apply patch
> with
> > last
> >     > minute changes.
> >     >
> >     > Cheers
> >     > Mike
> >     >
> >     >
> >     > On 22/02/2017, 00:56, "Jason Gustafson" <jason@confluent.io>
> wrote:
> >     >
> >     >     Hey Michael,
> >     >
> >     >     Awesome. I have a minor request. The APIs are currently
> > documented as a
> >     >     wiki list. Would you mind adding a code snippet instead? It's a
> > bit
> >     > easier
> >     >     to process.
> >     >
> >     >     How will be best to manage this, as we will obviously build off
> > your
> >     > KIP’s
> >     >     > protocol changes, to avoid a merge hell, should we branch
> from
> > your
> >     > branch
> >     >     > in the confluent repo or is it worth having a KIP-98 special
> > branch
> >     > in the
> >     >     > apache git, that we can branch/fork from?
> >     >
> >     >
> >     >     I was thinking about this also. Ideally we'd like to get the
> > changes
> >     > in as
> >     >     close together as possible since we only want one magic bump
> and
> > some
> >     > users
> >     >     deploy trunk. The level of effort to change the format for
> > headers
> >     > seems
> >     >     not too high. Do you agree? My guess is that the KIP-98 message
> > format
> >     >     patch will take 2-3 weeks to review before we merge to trunk,
> so
> > you
> >     > could
> >     >     hold off implementing until that patch has somewhat stabilized.
> > That
> >     > would
> >     >     save some potential rebase pain.
> >     >
> >     >     -Jason
> >     >
> >     >
> >     > 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.
> >     >
> >
> >
> > 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.
> >
> 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.
>

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