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" <[email protected]> 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" <[email protected]> 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 <[email protected]>
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 <[email protected]>
> Sent: Wednesday, February 22, 2017 10:09 PM
> To: [email protected]
> 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
<[email protected]>
> wrote:
>
> > So how would you have this work if not mutable where interceptors
would
> > add headers?
> >
> > Sent using OWA for iPhone
> > ________________________________________
> > From: Jason Gustafson <[email protected]>
> > Sent: Wednesday, February 22, 2017 8:42:27 PM
> > To: [email protected]
> > 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
<[email protected]>
> > 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, "[email protected] on behalf of Ismael
Juma" <
> > > [email protected] on behalf of [email protected]> 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 <
> > [email protected]
> > > >
> > > 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" <[email protected]>
> > 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.
