I feel it was a little odd to leave out the value length anyway, so I would rather add it back and put headers at the end. This is more consistent with the rest of the Kafka protocol.
-Jason On Tue, Feb 21, 2017 at 3:58 PM, Michael Pearce <michael.pea...@ig.com> wrote: > Or we keep as is (valuelen removed), and headers are added with headers > length.. > > On 21/02/2017, 23:38, "Apurva Mehta" <apu...@confluent.io> wrote: > > Right now, we don't need the value length: since it is the last item > in the > message, and we have the message length, we can deduce the value > length. > However, if we are adding record headers to the end, we would need to > introduce the value length along with that change. > > On Tue, Feb 21, 2017 at 3:34 PM, Michael Pearce <michael.pea...@ig.com > > > wrote: > > > It seems I cannot add comment on the doc. > > > > In the section around the message protocol. > > > > It has stated: > > > > Message => > > Length => uintVar > > Attributes => int8 > > TimestampDelta => intVar > > OffsetDelta => uintVar > > KeyLen => uintVar [OPTIONAL] > > Key => data [OPTIONAL] > > Value => data [OPTIONAL] > > > > Should it not be: (added missing value len) > > > > Message => > > Length => uintVar > > Attributes => int8 > > TimestampDelta => intVar > > OffsetDelta => uintVar > > KeyLen => uintVar [OPTIONAL] > > Key => data [OPTIONAL] > > ValueLen => uintVar [OPTIONAL] > > Value => data [OPTIONAL] > > > > > > > > On 21/02/2017, 23:07, "Joel Koshy" <jjkosh...@gmail.com> wrote: > > > > I left a couple of comments/questions directly on the google-doc > > <https://docs.google.com/document/d/11Jqy_ > > GjUGtdXJK94XGsEIK7CP1SnQGdp2eF0wSw9ra8> > > - I found it much more tractable for a proposal of this size to > > discuss in > > context within the doc. The permissions on the doc don't let > everyone > > view > > comments, so if there are any material changes that come out of > the > > discussions in those comment threads we can summarize here. > > > > Thanks, > > > > Joel > > > > On Mon, Feb 20, 2017 at 4:08 PM, Becket Qin < > becket....@gmail.com> > > wrote: > > > > > Thanks for the explanation, Guozhang. That makes sense. > > > > > > On Sun, Feb 19, 2017 at 7:28 PM, Guozhang Wang < > wangg...@gmail.com> > > wrote: > > > > > > > Thanks Becket. > > > > > > > > Actually sequence is associated with a message, not a > message set. > > For > > > > example if a message set sent by producer contains 100 > messages, > > and the > > > > first message's sequence is 5, then the last message's > sequence > > number > > > > would be 104, and the next message set's first sequence is > > expected to be > > > > 105. > > > > > > > > > > > > Guozhang > > > > > > > > > > > > On Sun, Feb 19, 2017 at 4:48 PM, Becket Qin < > becket....@gmail.com> > > > wrote: > > > > > > > > > +1. Thanks for the great work on the KIP! > > > > > > > > > > I have only one minor question, in the wiki (and the doc) > the new > > > message > > > > > set format has a "FirstSequence" field, should it just be > > "Sequence" if > > > > the > > > > > sequence is always associated with a message set? > > > > > > > > > > On Fri, Feb 17, 2017 at 3:28 AM, Michael Pearce < > > michael.pea...@ig.com > > > > > > > > > wrote: > > > > > > > > > > > +0 > > > > > > > > > > > > I think need some unified agreement on the VarInts. > > > > > > > > > > > > Would this also change in all other area’s of the > protocol, > > e.g. > > > value > > > > > and > > > > > > key length in message protocol, to keep this uniform > across all > > > > protocols > > > > > > going forwards? > > > > > > > > > > > > > > > > > > > > > > > > On 17/02/2017, 00:23, "Apurva Mehta" < > apu...@confluent.io> > > wrote: > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > Thanks for the reply. Comments inline. > > > > > > > > > > > > On Thu, Feb 16, 2017 at 2:29 PM, Jun Rao < > j...@confluent.io > > > > > > wrote: > > > > > > > > > > > > > Hi, Apurva, > > > > > > > > > > > > > > Thanks for the reply. A couple of comment below. > > > > > > > > > > > > > > On Wed, Feb 15, 2017 at 9:45 PM, Apurva Mehta < > > > > apu...@confluent.io > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > > > > > Answers inline: > > > > > > > > > > > > > > > > 210. Pid snapshots: Is the number of pid snapshot > > > configurable > > > > or > > > > > > > hardcoded > > > > > > > > > with 2? When do we decide to roll a new > snapshot? > > Based on > > > > > time, > > > > > > byte, > > > > > > > or > > > > > > > > > offset? Is that configurable too? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > When a replica becomes a follower, we do a bit log > > truncation. > > > > > > Having an > > > > > > > older snapshot allows us to recover the > PID->sequence > > mapping > > > > much > > > > > > quicker > > > > > > > than rescanning the whole log. > > > > > > > > > > > > > > > > > > This is a good point. I have updated the doc with a > more > > detailed > > > > > > proposal. > > > > > > Essentially, snapshots will be created on a periodic > > basis. A > > > > > > reasonable > > > > > > period would be every 30 or 60 seconds. We will keep > at > > most 2 > > > > copies > > > > > > of > > > > > > the snapshot file. With this setup, we would have to > > replay at > > > most > > > > > 60 > > > > > > or > > > > > > 120 seconds of the log in the event of log truncation > > during > > > leader > > > > > > failover. > > > > > > > > > > > > If we need to make any of this configurable, we can > expose > > a > > > config > > > > > in > > > > > > the > > > > > > future. It would be easier to add a config we need > than > > remove > > > one > > > > > with > > > > > > marginal utility. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 211. I am wondering if we should store > > ExpirationTime in > > > the > > > > > > producer > > > > > > > > > transactionalId mapping message as we do in the > > producer > > > > > > transaction > > > > > > > > status > > > > > > > > > message. If a producer only calls > > initTransactions(), but > > > > never > > > > > > > publishes > > > > > > > > > any data, we still want to be able to expire > and > > remove the > > > > > > producer > > > > > > > > > transactionalId mapping message. > > > > > > > > > > > > > > > > > > > > > > > > > > Actually, the document was inaccurate. The > > transactionalId > > > will > > > > > be > > > > > > > expired > > > > > > > > only if there is no active transaction, and the > age of > > the > > > last > > > > > > > transaction > > > > > > > > with that transactionalId is older than the > > transactioanlId > > > > > > expiration > > > > > > > > time. With these semantics, storing the > expiration > > time in > > > the > > > > > > > > transactionalId mapping message won't be useful, > since > > the > > > > > > expiration > > > > > > > time > > > > > > > > is a moving target based on transaction activity. > > > > > > > > > > > > > > > > I have updated the doc with a clarification. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Currently, the producer transactionalId mapping > message > > doesn't > > > > > carry > > > > > > > ExpirationTime, but the producer transaction status > > message > > > does. > > > > > > It would > > > > > > > be useful if they are consistent. > > > > > > > > > > > > > > > > > > > > You are right. The document has been updated to > remove the > > > > > > ExpirationTime > > > > > > from the transaction status messages as well. Any > utility > > for > > > this > > > > > > field > > > > > > can be achieved by using the timestamp of the message > > itself > > > along > > > > > with > > > > > > another expiration time (like transactionalId > expiration > > time, > > > > > > transaction > > > > > > expiration time, etc.). > > > > > > > > > > > > Thanks, > > > > > > Apurva > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > -- Guozhang > > > > > > > > > > > > > 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. >
