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.
