Hi Ismael,
Sorry,
The response below was in regards to your comments, got my wires crossed,
apologies.
Hi Jun,
I’m happy with the change, I see Jason updated our KIP, many thanks for this,
and thanks for implementing for us ☺
Cheers
Mike
On 20/03/2017, 13:19, "Michael Pearce" <michael.pea...@ig.com> wrote:
Hi Jun,
Thanks the comments I’ve updated the KIP a little where agreement.
My comments:
1) Good point, removed from the interface. See updated KIP
2) I think, Radai’s suggested header(String key) is a cleaner method name,
but happy to change if community believe lastHeader is better. I’ll keep
Radai’s suggested name for now.
3) Agreed, added IllegalStateException to interface. See updated KIP
4) I think for now intent would be to simply instantiate a new object from
the provided implementation that would exist and implement the Header interface
e.g. I would expect user to invoke “new HeaderRecord(String key, byte[] value)”
for now.
5) Agreed, we should add later if useful. I think as per other arguments of
trying to keep the methods for now limited as easier to add new methods later,
but cannot take away, as such I think we should avoid adding this, as you note
it is still possible with the current suggested api.
Cheers
Mike
On 17/03/2017, 13:47, "isma...@gmail.com on behalf of Ismael Juma"
<isma...@gmail.com on behalf of ism...@juma.me.uk> wrote:
Jun, the message format you proposed seems reasonable to me. I have a
few
minor comments with regards to the user facing API:
1. Do we want to expose the `close()` method in the Headers interface?
It
seems that this method should only be called by the producer after the
headers have been passed to the interceptors, so it may make sense to
keep
it as an internal method in the implementation class.
2. `header(String key)` returns the last header for that key. Maybe we
should make it explicit by calling the method `lastHeader(String key)`.
3. I agree with the change to throw an exception if we try to modify the
headers when they are in read-only mode. We should specify the
exception in
the KIP. IllegalStateException, as suggested by Radai, seems reasonable.
4. How do users create a `Header` instance to pass to the `add` method?
We
could introduce a static `create` method that takes both parameters to
the
`Header` interface (requires Java 8).
5. There's no method to replace all the headers with a given key so one
would have to call `remove` and then `add`. Is the assumption that this
is
rare? If so, that's probably OK, we can add another method later, if
it's
useful.
Thanks,
Ismael
On Thu, Mar 16, 2017 at 4:44 PM, Jun Rao <j...@confluent.io> wrote:
> Hi, Everyone,
>
> Jason has been working on the new message format related to EOS (
> https://github.com/apache/kafka/pull/2614). He has included the header
> changes proposed in the KIP, which reduces the overhead for
supporting an
> additional message format change if done separately. Since the message
> format part of the header proposal seems less controversial and the
> consensus is header is needed, does anyone have objections to this?
The
> following is the new record format with headers.
>
> * Record =>
> * Length => Varint
> * Attributes => Int8
> * TimestampDelta => Varlong
> * OffsetDelta => Varint
> * Key => Bytes
> * Value => Bytes
> * Headers => [HeaderKey HeaderValue]
> * HeaderKey => String
> * HeaderValue => Bytes
> *
> * Note that in this schema, the Bytes and String types use a variable
> length integer to represent
> * the length of the field. The array type used for the headers also
> uses a Varint for the number of
> * headers.
>
> Thanks,
>
> Jun
>
>
> On Tue, Mar 14, 2017 at 10:49 AM, Ismael Juma <ism...@juma.me.uk>
wrote:
>
> > Thanks Radai. Great to have a concrete example of the intended
usage.
> >
> > Regarding performance, we would need to benchmark, as you said. But
there
> > would be a lot of reuse (in essence, we are copying 5 references
plus a
> new
> > object header), so I'd be surprised if that would be the bottleneck
> > compared to some of the other allocations that would be happening
in that
> > path. In any case, I think we can leave this aside for now since
people
> > also felt that the mutable API would be easier to use.
> >
> > About ProducerRecord reuse, my understanding is that people do
sometimes
> > retry a failed request manually due to the fact that a large retry
number
> > doesn't help if a batch is expired in the queue. I believe KIP-91
will
> > help:
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 91+Provide+Intuitive+User+Timeouts+in+The+Producer
> >
> > In addition, KIP-98 (Exactly-once) won't achieve its goal if people
do
> > manual retries. So, it seems like it's OK to require people to
create a
> new
> > ProducerRecord if they really need to do manual retries. But we
should
> add
> > a note to the compatibility section of the KIP.
> >
> > I have a few minor API suggestions. I'll send a follow-up later
today,
> > hopefully.
> >
> > Ismael
> >
> > On Mon, Mar 13, 2017 at 6:23 PM, radai <radai.rosenbl...@gmail.com>
> wrote:
> >
> > > the common "stack" we envision at linkedin would consist of (at
least)
> > the
> > > following components that add headers to every outgoing request:
> > >
> > > 1. auditing/"lineage" - appends a header containing "node"
(hostname
> > etc),
> > > time (UTC time) and destination (cluster/topic). these accumulate
as
> > > requests get mirrored between clusters
> > > 2. serialization - sets a header containing a schema identifier to
> allow
> > > deserialization
> > > 3. client-side encryption - would probably set a header
identifying the
> > > key/scheme used
> > > 4. internal "billing"
> > >
> > > there are also several other teams at linkedin that would use
headers
> > > (although its unclear yet if via interceptors or by directly
> manipulating
> > > requests)
> > >
> > > if headers are made completely immutable (as the entire request
object
> > > currently is) we would end up copying (parts of) every msg 4
times. I
> > > havent benchmarked but this seems like it would have an impact to
me.
> > >
> > > looking elsewhere rabbitMQ and http components both use mutable
request
> > > objects (rabbitMW's BasicProperties object, http components'
addHeader
> > > method).
> > >
> > > how common is it right now for instances of ProducerRecord to
actually
> be
> > > reused?
> > > do people really have things like publis static final
ProducerRecord
> > > MY_FAVORITE_REQUEST = ... ?
> > >
> >
>
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.
