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. >
