Hey Michael, Hmm, I guess the point of representing it as bytes is to allow the broker to pass it through opaquely? Is the cost of parsing them a concern, or are we simply trying to ensure that the broker stays agnostic to the format?
On varints, I think adding support for them makes less sense for an isolated use case, but as part of a more holistic change (such as what we have proposed in KIP-98), I think they are justifiable. If we add them, then the need to use attributes becomes quite a bit weaker, right? The other thing I find slightly odd is the fact that null headers has no actual semantic meaning for the message (unlike null keys and values). It is just a space optimization. It seems a bit better to always use size 0 to indicate having no headers. Overall, the main point is ensuring that the message schema remains consistent, either within the larger protocol, or at a minimum within the message itself. -Jason On Thu, Feb 16, 2017 at 6:39 AM, Michael Pearce <michael.pea...@ig.com> wrote: > Hi Jason, > > On point 1) in the message protocol the headers are simply a byte array, > as like the key or value, this is to clearly demarcate the header in the > core message. Then the header byte array in the core message is an array of > key, value pairs. This is what it is denoting. > > Then this would be I guess in the given notation: > > Headers => [KeyLength, Key, ValueLength, Value] > KeyLength => int32 <-----------------NEW size of the byte[] of the > serialised key value > Key => bytes <---------------------- NEW serialised string (UTF8) > bytes of the header key > ValueLength => int32 <-------------- NEW size of the byte[] of the > serialised header value > Value => bytes <-------------------- NEW serialised form of the header > value > > The key length and value length is matching the way the protocol is > defined in the core message currently. > > > > > On point 2) > Var sized ints, this was discussed much earlier on, in fact I had > suggested it myself (with Hadoop references), the complexity of this > compared to having a simpler protocol was argued and agreed it wasn’t worth > the complexity as all other clients in other languages would need to ensure > theyre using the right var size algorithm, as there is a few. > > On point 3) > We did the attributes, optional approach as originally there was marked > concern that headers would cause a message size overhead for others, who > don’t want them. As such this is the clean solution to achieve that. If > that no longer holds, and we don’t care that we add 4bytes overhead, then > im happy to remove. > > I’m personally in favour of keeping the message as small as possible so > people don’t get shocks in perf and throughputs dues to message size, > unless they actively use the feature, as such I do prefer the attribute bit > wise feature flag approach myself. > > > > > On 16/02/2017, 05:40, "Jason Gustafson" <ja...@confluent.io> wrote: > > We have proposed a few significant changes to the message format in > KIP-98 > which now seems likely to pass (perhaps with some iterations on > implementation details). It would be good to try and coordinate the > changes > in both of the proposals to make sure they are consistent and > compatible. > > I think using the attributes to indicate null headers is a reasonable > approach. We have proposed to do the same thing for the message key and > value. That said, I sympathize with Jay's argument. Having multiple > ways to > specify a null value increases the overall complexity of the protocol. > You > can see this just from the fact that you need the extra verbiage in the > protocol specification in this KIP and in KIP-98 to describe the > dependence > between the fields and the attributes. It seems like a slippery slope > if > you start allowing different request types to implement the protocol > specification differently. > > You can also argue that the messages already are and are likely to > remain a > special case. For example, there is currently no generality in how > compressed message sets are represented that would be applicable for > other > request types. Some might see this divergence as an unfortunate > protocol > deficiency which should be fixed; others might see it as sort of the > inevitability of needing to optimize where it counts most. I'm probably > somewhere in between, but I think we probably all share the intuition > that > the protocol should be kept as consistent as possible. With that in > mind, > here are a few comments: > > 1. One thing I found a little odd when reading the current proposal is > that > the headers are both represented as an array of bytes and as an array > of > key/value pairs. I'd probably suggest something like this: > > Headers => [HeaderKey HeaderValue] > HeaderKey => String > HeaderValue => Bytes > > An array in the Kafka protocol is represented as a 4-byte integer > indicating the number of elements in the array followed by the > serialization of the elements. Unless I'm misunderstanding, what you > have > instead is the total size of the headers in bytes followed by the > elements. > I'm not sure I see any reason for this inconsistency. > > 2. In KIP-98, we've introduced variable-length integer fields. > Effectively, > we've enriched (or "complicated" as Jay might say ;) the protocol > specification to include the following types: VarInt, VarLong, > UnsignedVarInt and UnsignedVarLong. > > Along with these primitives, we could introduce the following types: > > VarSizeArray => NumberOfItems Item1 Item2 .. ItemN > NumberOfItems => UnsignedVarInt > > VarSizeNullableArray => NumberOfItemsOrNull Item1 Item2 .. ItemN > NumberOfItemsOrNull => VarInt (-1 means null) > > And similarly for the `String` and `Bytes` types. These types can save > a > considerable amount of space in this proposal because they can be used > for > both the number of headers included in the message and the lengths of > the > header keys and values. We could do this instead: > > Headers => VarSizeArray[HeaderKey HeaderValue] > HeaderKey => VarSizeString > HeaderValue => VarSizeBytes > > Combining the savings from the use of variable length fields, the > benefit > of using the attributes to represent null seems pretty small. > > 3. Whichever way we go (whether we use the attributes or not), we > should at > least be consistent between this KIP and KIP-98. It would be very > strange > to have two ways to represent null values in the same schema. Either > way is > OK with me. I think some message-level optimizations are justifiable, > but > the savings here seem minimal (a few bytes per message), so maybe it's > not > worth the cost of letting the message diverge even further from the > rest of > the protocol. > > -Jason > > > On Wed, Feb 15, 2017 at 8:52 AM, radai <radai.rosenbl...@gmail.com> > wrote: > > > I've trimmed the inline contents as this mail is getting too big for > the > > apache mailing list software to deliver :-( > > > > 1. the important thing for interoperability is for different > "interested > > parties" (plugins, infra layers/wrappers, user-code) to be able to > stick > > pieces of metadata onto msgs without getting in each other's way. a > common > > key scheme (Strings, as of the time of this writing?) is all thats > required > > for that. it is assumed that the other end interested in any such > piece of > > metadata knows the encoding, and byte[] provides for the most > flexibility. > > i believe this is the same logic behind core kafka being > byte[]/byte[] - > > Strings are more "usable" but bytes are flexible and so were chosen. > > Also - core kafka doesnt even do that good of a job on usability of > the > > payload (example - i have to specify the nop byte[] "decoders" > explicitly > > in conf), and again sacrificies usability for the sake of > performance (no > > convenient single-record processing as poll is a batch, lots of > obscure > > little config details exposing internals of the batching mechanism, > etc) > > > > this is also why i really dislike the idea of a "type system" for > header > > values, it further degrades the usability, adds complexity and will > > eventually get in people's way, also, it would be the 2nd/3rd > home-group > > serialization mechanism in core kafka (counting 2 iterations of the > "type > > definition DSL") > > > > 2. this is an implementation detail, and not even a very "user > facing" one? > > to the best of my understanding the vote process is on proposed > > API/behaviour. also - since we're willing to go with strings just > serialize > > a 0-sized header blob and IIUC you dont need any optionals anymore. > > > > 3. yes, we can :-) > > > > On Tue, Feb 14, 2017 at 11:56 PM, Michael Pearce < > michael.pea...@ig.com> > > wrote: > > > > > Hi Jay, > > > > > > 1) There was some initial debate on the value part, as youll note > String, > > > String headers were discounted early on. The reason for this is > > flexibility > > > and keeping in line with the flexibility of key, value of the > message > > > object itself. I don’t think it takes away from an ecosystem as > each > > plugin > > > will care for their own key, this way ints, booleans , exotic > custom > > binary > > > can all be catered for=. > > > a. If you really wanted to push for a typed value interface, I > wouldn’t > > > want just String values supported, but the the primatives plus > string and > > > also still keeping the ability to have a binary for custom > binaries that > > > some organisations may have. > > > i. I have written this slight alternative here, > > https://cwiki.apache.org/ > > > confluence/display/KAFKA/KIP-82+-+Add+Record+Headers+-+Typed > > > ii. Essentially the value bytes, has a leading byte overhead. > > > 1. This tells you what type the value is, before reading the rest > of the > > > bytes, allowing serialisation/deserialization to and from the > primitives, > > > string and byte[]. This is akin to some other messaging systems. > > > 2) We are making it optional, so that for those not wanting > headers have > > 0 > > > bytes overhead (think of it as a feature flag), I don’t think this > is > > > complex, especially if comparing to changes proposed in other kips > like > > > kip-98. > > > a. If you really really don’t like this, we can drop it, but it > would > > mean > > > buying into 4 bytes extra overhead for users who do not want to use > > headers. > > > 3) In the summary yes, it is at a higher level, but I think this > is well > > > documented in the proposed changes section. > > > a. Added getHeaders method to Producer/Consumer record (that is it) > > > b. We’ve also detailed the new Headers class that this method > returns > > that > > > encapsulates the headers protocol and logic. > > > > > > Best, > > > Mike > > > > > > ==Original questions from the vote thread from Jay.== > > > > > > Couple of things I think we still need to work out: > > > > > > 1. I think we agree about the key, but I think we haven't > talked about > > > the value yet. I think if our goal is an open ecosystem of these > > header > > > spread across many plugins from many systems we should consider > making > > > this > > > a string as well so it can be printed, set via a UI, set in > config, > > etc. > > > Basically encouraging pluggable serialization formats here will > lead > > to > > > a > > > bit of a tower of babel. > > > 2. This proposal still includes a pretty big change to our > > serialization > > > and protocol definition layer. Essentially it is introducing an > > optional > > > type, where the format is data dependent. I think this is > actually a > > big > > > change though it doesn't seem like it. It means you can no > longer > > > specify > > > this type with our type definition DSL, and likewise it requires > > custom > > > handling in client libs. This isn't a huge thing, since the > Record > > > definition is custom anyway, but I think this kind of protocol > > > inconsistency is very non-desirable and ties you to hand-coding > > things. > > > I > > > think the type should instead by [Key Value] in our BNF, where > key and > > > value are both short strings as used elsewhere. This brings it > in line > > > with > > > the rest of the protocol. > > > 3. Could we get more specific about the exact Java API change to > > > ProducerRecord, ConsumerRecord, Record, etc? > > > > > > -Jay > > > > > > > > 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. >
