append-only would mean that if (for whatever reason) i want to replace a
header or strip it out i'd need to copy the whole record.
On Wed, Feb 22, 2017 at 5:09 PM, Michael Pearce <michael.pea...@ig.com>
wrote:
> Im happy to compromise to keep it mutable but move to an append style api.
> (as in guava interables concat)
>
> class Headers {
> Headers append(Iterable<Header> headers);
> }
>
>
> I don’t think we’d want prepend, this would give the idea of guaranteed
> ordering, when in actual fact we don’t provide that guarantee (.e.g one
> client can put headerA, then headerB, but another could put headerB then
> headerA, this shouldn’t cause issues), Also what if we changed to a hashmap
> for the internal implementation, its just a bucket of entries no ordering.
> I think we just need to provide an api to add/append headers.
>
> This ok? If so ill update KIP to record this.
>
> Cheers
> Mike
>
> On 23/02/2017, 00:37, "Jason Gustafson" <ja...@confluent.io> wrote:
>
> The point about usability is fair. It's also reasonable to expect that
> common use cases such as appending headers should be done efficiently.
>
> Perhaps we could compromise with something like this?
>
> class Headers {
> Headers append(Iterable<Header> headers);
> Headers prepend(Iterable<Header> headers);
> }
>
> That retains ease of use while still giving ourselves some flexibility
> in
> the implementation.
>
> -Jason
>
>
> On Wed, Feb 22, 2017 at 3:03 PM, Michael Pearce <michael.pea...@ig.com
> >
> wrote:
>
> > I wasn’t referring to the headers needing to be copied, im meaning
> the
> > fact we’d be forcing a new producer record to be created, with all
> the
> > contents copied.
> >
> > i.e what will happen is utility method will be created or end up
> being
> > used, which does this, and returns the new ProducerRecord instance.
> >
> > ProducerRecord addHeader(ProducerRecord record, Header header){
> > Return New ProducerRecord(record.key, record.value,
> record.timestamp…..,
> > record.headers.concat(header))
> > }
> >
> > To me this seems ugly, but will be inevitable if we don’t make adding
> > headers to existing records a simple clean method call.
> >
> >
> >
> > On 22/02/2017, 22:57, "Michael Pearce" <michael.pea...@ig.com>
> wrote:
> >
> > Lazy init can achieve/avoid that.
> >
> > Re the concat, why don’t we implement that inside the Headers
> rather
> > than causing everyone to implement this as adding headers in
> interceptors
> > will be a dominant use case. We want a user friendly API. Having as
> a user
> > having to code this instead of having the headers handle this for me
> seems
> > redundant.
> >
> > On 22/02/2017, 22:34, "Jason Gustafson" <ja...@confluent.io>
> wrote:
> >
> > I thought the argument was against creating the extra objects
> > unnecessarily
> > (i.e. if they were not accessed). And note that making the
> Headers
> > immutable doesn't necessarily mean that they need to be
> copied:
> > you can do
> > a trick like Guava's Iterables.concat to add additional
> headers
> > without
> > changing the underlying collections.
> >
> > -Jason
> >
> > On Wed, Feb 22, 2017 at 2:22 PM, Michael Pearce <
> > michael.pea...@ig.com>
> > wrote:
> >
> > > If the argument for not having a map holding the key, value
> > pairs is due
> > > to garbage creation of HashMap entry's, forcing the
> creation of
> > a whole new
> > > producer record to simply add a head, surely is creating
> a-lot
> > more?
> > > ________________________________________
> > > From: Jason Gustafson <ja...@confluent.io>
> > > Sent: Wednesday, February 22, 2017 10:09 PM
> > > To: dev@kafka.apache.org
> > > Subject: Re: [DISCUSS] KIP-82 - Add Record Headers
> > >
> > > 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.pea...@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 <ja...@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.pea...@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, "isma...@gmail.com on behalf of
> > Ismael Juma" <
> > > > > isma...@gmail.com on behalf of ism...@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.pea...@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" <
> > ja...@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.
> > > >
> > > 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.
>
