Hi Ismael, Thanks for the reply. Please see the comments inline.
On Wed, Mar 1, 2017 at 6:47 AM, Ismael Juma <ism...@juma.me.uk> wrote: > Hi Becket, > > Thanks for sharing your thoughts. More inline. > > On Wed, Mar 1, 2017 at 2:54 AM, Becket Qin <becket....@gmail.com> wrote: > > > As you can imagine if the ProducerRecord has a value as a List and the > > Interceptor.onSend() can actually add an element to the List. If the > > producer.send() is called on the same ProducerRecord again, the value > list > > would have one more element than the previously sent ProducerRecord even > > though the ProducerRecord itself is not mutable, right? Same thing can > > apply to any modifiable class type. > > > > The difference is that the user chooses the value type. They are free to > choose a mutable or immutable type. A generic interceptor cannot mutate the > value because it doesn't know the type (and it could be immutable). One > could write an interceptor that checked the type of the value at runtime > and did things based on that. But again, the user who creates the record is > in control. > But there is no generic interceptor, right? The interceptor always takes specific K, V type. > From this standpoint allowing headers to be mutable doesn't really weaken > > the mutability we already have. Admittedly a mutable header is kind of > > guiding user towards to change the headers in the existing object instead > > of creating a new one. > > > Yes, with headers, we are providing a model for the user (the user doesn't > get to choose it like with keys and values) and for the interceptors. So, I > think it's not the same. > > > But I think reusing an object while it is possible > > to be modified by user code is a risk that users themselves are willing > to > > take. And we do not really protect against that. > > > If users want to take that risk, it's fine. But we give them the option to > protect themselves. With mutable headers, there is no option. If we want to let the users control the mutability, users can always call headers.close() before calling producer.send() and that will force the interceptor to create new ProducerRecord object. Because the headers are mostly useful for interceptors, unless the users do not want the interceptors to change their records, it seems reasonable to say that by default modification of headers are allowed for the interceptors. > > > > But there still seems > > value to allow the users to not pay the overhead of creating tons of > > objects if they do not reuse an object to send it twice, which is > probably > > a more common case. > > > > I think the assumption that there would be tons of objects created is > incorrect (I suggested a solution that would only involve one additional > reference in the `Header` instance). The usability of the immutable API > seemed to be more of an issue. > If we do not allow the users to add headers on existing ProducerRecord objects, each interceptor who wants to add headers will have to create a new ProducerRecord. So we will have to create NUM_INTERCEPTOR times of ProducerRecord, if a producer is sending 100K messages per second, it would be a lot of new objects, right? > > In any case, if we do add the `close()` method, then we need to add a note > to the compatibility section stating that once a producer record is sent, > it cannot be sent again as this would cause interceptors that add headers > to fail. > Agreed, clear documentation is important. > > Ismael >
