So currently KIP has:
append
filter (get by key, but not inferring perf as noted expectation on O(1) perf
with a get method)
I agree that supporting some form of
remove
replace
is standard in this kind of API and for the benefit of making a usable API, we
should explore adding these.
If we’re avoid the put, get styled interface as there seems contention with
this naming, we happy with naming these as I mentioned?
On 23/02/2017, 12:16, "radai" <radai.rosenbl...@gmail.com> wrote:
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.
>
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.
