Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Colin McCabe <cm...@apache.org>]

Hi all,

I've made some updates to this KIP.  Specifically, I wanted to avoid including escape bytes in the serialization format, since it was too complex.  Also, I think this is a good opportunity to slim down our variable length fields.

best,
Colin


On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > Thanks Colin for the KIP. For my own edification why are we doing this
> > "Optional fields can have any type, except for an array of structures."?
> > Why can't we have an array of structures?
> 
> Optional fields are serialized starting with their total length.  This 
> is straightforward to calculate for primitive fields like INT32, (or 
> even an array of INT32), but more difficult to calculate for an array 
> of structures.  Basically, we'd have to do a two-pass serialization 
> where we first calculate the lengths of everything, and then write it 
> out.
> 
> The nice thing about this KIP is that there's nothing in the protocol 
> stopping us from adding support for this feature in the future.  We 
> wouldn't have to really change the protocol at all to add support.  But 
> we'd have to change a lot of serialization code.  Given almost all of 
> our use-cases for optional fields are adding an extra field here or 
> there, it seems reasonable not to support it for right now.
> 
> best,
> Colin
> 
> > 
> > -- 
> > -Jose
> >
>

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Colin McCabe <cm...@apache.org>]

On Tue, Aug 13, 2019, at 11:19, David Jacot wrote:
> Hi Colin,
> 
> Thank you for the KIP! Things are well explained!. It is huge improvement
> for the Kafka protocol. I have few comments on the proposal:
> 
> 1. The interleaved tag/length header sounds like a great optimisation as it
> would be shorter on average. The downside, as
> you already pointed out, is that it makes the decoding and the specs more
> complex. Personally, I would also favour using two
> vaints in this particular case to keep things simple.

Hi David,

Thanks for the review.

I changed this to be two separate unsigned varints, as you and Jason suggested.  The extra complexity is just probably not worth it to save a byte here.  Using two varints also saves space if the length of the tag and the length of the size are not similar in size (i.e. it improves the worst case scenario).

> 
> 2. As discussed, I wonder if it would make sense to extend to KIP to also
> support optional fields in the Record Header. I think
> that it could be interesting to have such capability for common fields
> across all the requests or responses (e.g. tracing id).

Yeah, I think this is a great idea.  I added a section about updating to a new version of the request header and response header for message versions in flexibleVersions.  This will give us the ability to add optional stuff to the headers when needed in the future.  For things that span all requests, like ClientType, TraceId, etc., this will be very useful.

best,
Colin

> 
> Regards,
> David
> 
> 
> 
> On Tue, Aug 13, 2019 at 10:00 AM Jason Gustafson <ja...@confluent.io> wrote:
> 
> > > Right, I was planning on doing exactly that for all the auto-generated
> > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > better use of time to convert the manual ones to auto gen first (with the
> > possible exception of Fetch/Produce, where the ROI may be higher for the
> > manual work)
> >
> > Yeah, that makes sense. Maybe we can include the version bump for all RPCs
> > in this KIP, but we can implement it lazily as the protocols are converted.
> >
> > -Jason
> >
> > On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > > Hi Colin,
> > > >
> > > > Thanks for the KIP! This is a significant improvement. One of my
> > personal
> > > > interests in this proposal is solving the compatibility problems we
> > have
> > > > with the internal schemas used to define consumer offsets and
> > transaction
> > > > metadata. Currently we have to guard schema bumps with the inter-broker
> > > > protocol format. Once the format is bumped, there is no way to
> > downgrade.
> > > > By fixing this, we can potentially begin using the new schemas before
> > the
> > > > IBP is bumped while still allowing downgrade.
> > > >
> > > > There are a surprising number of other situations we have encountered
> > > this
> > > > sort of problem. We have hacked around it in special cases by allowing
> > > > nullable fields to the end of the schema, but this is not really an
> > > > extensible approach. I'm looking forward to having a better option.
> > >
> > > Yeah, this problem keeps coming up.
> > >
> > > >
> > > > With that said, I have a couple questions on the proposal:
> > > >
> > > > 1. For each request API, we need one version bump to begin support for
> > > > "flexible versions." Until then, we won't have the option of using
> > tagged
> > > > fields even if the broker knows how to handle them. Does it make sense
> > to
> > > > go ahead and do a universal bump of each request API now so that we'll
> > > have
> > > > this option going forward?
> > >
> > > Right, I was planning on doing exactly that for all the auto-generated
> > > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > > better use of time to convert the manual ones to auto gen first (with the
> > > possible exception of Fetch/Produce, where the ROI may be higher for the
> > > manual work)
> > >
> > > > 2. The alternating length/tag header encoding lets us save a byte in
> > the
> > > > common case. The downside is that it's a bit more complex to specify.
> > It
> > > > also has some extra cost if the length exceeds the tag substantially.
> > > > Basically we'd have to pad the tag, right? I think I'm wondering if we
> > > > should just bite the bullet and use two varints instead.
> > >
> > > That’s a fair point. It would be shorter on average, but worse for some
> > > exceptional cases. Also, the decoding would be more complex, which might
> > be
> > > a good reason to go for just having two varints. Yeah, let’s simplify.
> > >
> > > Regards,
> > > Colin
> > >
> > > >
> > > > Thanks,
> > > > Jason
> > > >
> > > >
> > > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <cm...@apache.org>
> > wrote:
> > > >
> > > > > Hi all,
> > > > >
> > > > > I've made some updates to this KIP. Specifically, I wanted to avoid
> > > > > including escape bytes in the serialization format, since it was too
> > > > > complex. Also, I think this is a good opportunity to slim down our
> > > > > variable length fields.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > > > > > > Thanks Colin for the KIP. For my own edification why are we doing
> > > this
> > > > > > > "Optional fields can have any type, except for an array of
> > > > > structures."?
> > > > > > > Why can't we have an array of structures?
> > > > > >
> > > > > > Optional fields are serialized starting with their total length.
> > This
> > > > > > is straightforward to calculate for primitive fields like INT32,
> > (or
> > > > > > even an array of INT32), but more difficult to calculate for an
> > array
> > > > > > of structures. Basically, we'd have to do a two-pass serialization
> > > > > > where we first calculate the lengths of everything, and then write
> > it
> > > > > > out.
> > > > > >
> > > > > > The nice thing about this KIP is that there's nothing in the
> > protocol
> > > > > > stopping us from adding support for this feature in the future. We
> > > > > > wouldn't have to really change the protocol at all to add support.
> > > But
> > > > > > we'd have to change a lot of serialization code. Given almost all
> > of
> > > > > > our use-cases for optional fields are adding an extra field here or
> > > > > > there, it seems reasonable not to support it for right now.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > -Jose
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Colin McCabe <cm...@apache.org>]

Hi David,

Good point.  I think it makes the most sense to put the tagged fields sections at the end of the header.  For consistency's sake, we can also put them at the end of the structures / requests / responses as well.  I've updated the KIP.

best,
Colin

On Mon, Aug 19, 2019, at 02:23, David Jacot wrote:
> Hi Colin,
> 
> Thank you for the updated KIP.
> 
> wrt. Request and Response Headers v1, where do you plan to put the optional
> fields? You mention that all the Requests and Responses will start with a
> tagged field buffer. Is it for that purpose?
> 
> Best,
> David
> 
> On Mon, Aug 19, 2019 at 7:27 AM Satish Duggana <sa...@gmail.com>
> wrote:
> 
> > Please read struct type as a complex record type in my earlier mail.
> > The complex type seems to be defined as Schema[1] in the protocol
> > types.
> >
> > 1.
> > https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java#L27
> >
> >
> > On Mon, Aug 19, 2019 at 9:46 AM Satish Duggana <sa...@gmail.com>
> > wrote:
> > >
> > > Sorry! Colin, I may not have been clear in my earlier query about
> > > optional field type restriction. It is mentioned in one of your
> > > replies "optional fields are serialized starting with their total
> > > length". This brings the question of whether optional fields support
> > > struct types (with or without array values). It seems struct types are
> > > currently not serialized with total length. I may be missing something
> > > here.
> > >
> > > Thanks,
> > > Satish.
> > >
> > >
> > > On Wed, Aug 14, 2019 at 8:03 AM Satish Duggana <sa...@gmail.com>
> > wrote:
> > > >
> > > > Hi Colin,
> > > > Thanks for the KIP. Optional fields and var length encoding support is
> > a great
> > > > improvement for the protocol.
> > > >
> > > > >>Optional fields can have any type, except that they cannot be arrays.
> > > > Note that the restriction against having tagged arrays is just to
> > simplify
> > > > serialization.  We can relax this restriction in the future without
> > changing
> > > > the protocol on the wire.
> > > >
> > > > Can an Optional field have a struct type which internally contains an
> > array
> > > > field at any level?
> > > >
> > > > Thanks,
> > > > Satish.
> > > >
> > > >
> > > >
> > > > On Tue, Aug 13, 2019 at 11:49 PM David Jacot <dj...@confluent.io>
> > wrote:
> > > > >
> > > > > Hi Colin,
> > > > >
> > > > > Thank you for the KIP! Things are well explained!. It is huge
> > improvement
> > > > > for the Kafka protocol. I have few comments on the proposal:
> > > > >
> > > > > 1. The interleaved tag/length header sounds like a great
> > optimisation as it
> > > > > would be shorter on average. The downside, as
> > > > > you already pointed out, is that it makes the decoding and the specs
> > more
> > > > > complex. Personally, I would also favour using two
> > > > > vaints in this particular case to keep things simple.
> > > > >
> > > > > 2. As discussed, I wonder if it would make sense to extend to KIP to
> > also
> > > > > support optional fields in the Record Header. I think
> > > > > that it could be interesting to have such capability for common
> > fields
> > > > > across all the requests or responses (e.g. tracing id).
> > > > >
> > > > > Regards,
> > > > > David
> > > > >
> > > > >
> > > > >
> > > > > On Tue, Aug 13, 2019 at 10:00 AM Jason Gustafson <ja...@confluent.io>
> > wrote:
> > > > >
> > > > > > > Right, I was planning on doing exactly that for all the
> > auto-generated
> > > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s
> > probably a
> > > > > > better use of time to convert the manual ones to auto gen first
> > (with the
> > > > > > possible exception of Fetch/Produce, where the ROI may be higher
> > for the
> > > > > > manual work)
> > > > > >
> > > > > > Yeah, that makes sense. Maybe we can include the version bump for
> > all RPCs
> > > > > > in this KIP, but we can implement it lazily as the protocols are
> > converted.
> > > > > >
> > > > > > -Jason
> > > > > >
> > > > > > On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org>
> > wrote:
> > > > > >
> > > > > > > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > > > > > > Hi Colin,
> > > > > > > >
> > > > > > > > Thanks for the KIP! This is a significant improvement. One of
> > my
> > > > > > personal
> > > > > > > > interests in this proposal is solving the compatibility
> > problems we
> > > > > > have
> > > > > > > > with the internal schemas used to define consumer offsets and
> > > > > > transaction
> > > > > > > > metadata. Currently we have to guard schema bumps with the
> > inter-broker
> > > > > > > > protocol format. Once the format is bumped, there is no way to
> > > > > > downgrade.
> > > > > > > > By fixing this, we can potentially begin using the new schemas
> > before
> > > > > > the
> > > > > > > > IBP is bumped while still allowing downgrade.
> > > > > > > >
> > > > > > > > There are a surprising number of other situations we have
> > encountered
> > > > > > > this
> > > > > > > > sort of problem. We have hacked around it in special cases by
> > allowing
> > > > > > > > nullable fields to the end of the schema, but this is not
> > really an
> > > > > > > > extensible approach. I'm looking forward to having a better
> > option.
> > > > > > >
> > > > > > > Yeah, this problem keeps coming up.
> > > > > > >
> > > > > > > >
> > > > > > > > With that said, I have a couple questions on the proposal:
> > > > > > > >
> > > > > > > > 1. For each request API, we need one version bump to begin
> > support for
> > > > > > > > "flexible versions." Until then, we won't have the option of
> > using
> > > > > > tagged
> > > > > > > > fields even if the broker knows how to handle them. Does it
> > make sense
> > > > > > to
> > > > > > > > go ahead and do a universal bump of each request API now so
> > that we'll
> > > > > > > have
> > > > > > > > this option going forward?
> > > > > > >
> > > > > > > Right, I was planning on doing exactly that for all the
> > auto-generated
> > > > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s
> > probably a
> > > > > > > better use of time to convert the manual ones to auto gen first
> > (with the
> > > > > > > possible exception of Fetch/Produce, where the ROI may be higher
> > for the
> > > > > > > manual work)
> > > > > > >
> > > > > > > > 2. The alternating length/tag header encoding lets us save a
> > byte in
> > > > > > the
> > > > > > > > common case. The downside is that it's a bit more complex to
> > specify.
> > > > > > It
> > > > > > > > also has some extra cost if the length exceeds the tag
> > substantially.
> > > > > > > > Basically we'd have to pad the tag, right? I think I'm
> > wondering if we
> > > > > > > > should just bite the bullet and use two varints instead.
> > > > > > >
> > > > > > > That’s a fair point. It would be shorter on average, but worse
> > for some
> > > > > > > exceptional cases. Also, the decoding would be more complex,
> > which might
> > > > > > be
> > > > > > > a good reason to go for just having two varints. Yeah, let’s
> > simplify.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Colin
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Jason
> > > > > > > >
> > > > > > > >
> > > > > > > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <
> > cmccabe@apache.org>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi all,
> > > > > > > > >
> > > > > > > > > I've made some updates to this KIP. Specifically, I wanted
> > to avoid
> > > > > > > > > including escape bytes in the serialization format, since it
> > was too
> > > > > > > > > complex. Also, I think this is a good opportunity to slim
> > down our
> > > > > > > > > variable length fields.
> > > > > > > > >
> > > > > > > > > best,
> > > > > > > > > Colin
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > > > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio
> > wrote:
> > > > > > > > > > > Thanks Colin for the KIP. For my own edification why are
> > we doing
> > > > > > > this
> > > > > > > > > > > "Optional fields can have any type, except for an array
> > of
> > > > > > > > > structures."?
> > > > > > > > > > > Why can't we have an array of structures?
> > > > > > > > > >
> > > > > > > > > > Optional fields are serialized starting with their total
> > length.
> > > > > > This
> > > > > > > > > > is straightforward to calculate for primitive fields like
> > INT32,
> > > > > > (or
> > > > > > > > > > even an array of INT32), but more difficult to calculate
> > for an
> > > > > > array
> > > > > > > > > > of structures. Basically, we'd have to do a two-pass
> > serialization
> > > > > > > > > > where we first calculate the lengths of everything, and
> > then write
> > > > > > it
> > > > > > > > > > out.
> > > > > > > > > >
> > > > > > > > > > The nice thing about this KIP is that there's nothing in
> > the
> > > > > > protocol
> > > > > > > > > > stopping us from adding support for this feature in the
> > future. We
> > > > > > > > > > wouldn't have to really change the protocol at all to add
> > support.
> > > > > > > But
> > > > > > > > > > we'd have to change a lot of serialization code. Given
> > almost all
> > > > > > of
> > > > > > > > > > our use-cases for optional fields are adding an extra
> > field here or
> > > > > > > > > > there, it seems reasonable not to support it for right now.
> > > > > > > > > >
> > > > > > > > > > best,
> > > > > > > > > > Colin
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > -Jose
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> >
>

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[David Jacot <dj...@confluent.io>]

Hi Colin,

Thank you for the updated KIP.

wrt. Request and Response Headers v1, where do you plan to put the optional
fields? You mention that all the Requests and Responses will start with a
tagged field buffer. Is it for that purpose?

Best,
David

On Mon, Aug 19, 2019 at 7:27 AM Satish Duggana <sa...@gmail.com>
wrote:

> Please read struct type as a complex record type in my earlier mail.
> The complex type seems to be defined as Schema[1] in the protocol
> types.
>
> 1.
> https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java#L27
>
>
> On Mon, Aug 19, 2019 at 9:46 AM Satish Duggana <sa...@gmail.com>
> wrote:
> >
> > Sorry! Colin, I may not have been clear in my earlier query about
> > optional field type restriction. It is mentioned in one of your
> > replies "optional fields are serialized starting with their total
> > length". This brings the question of whether optional fields support
> > struct types (with or without array values). It seems struct types are
> > currently not serialized with total length. I may be missing something
> > here.
> >
> > Thanks,
> > Satish.
> >
> >
> > On Wed, Aug 14, 2019 at 8:03 AM Satish Duggana <sa...@gmail.com>
> wrote:
> > >
> > > Hi Colin,
> > > Thanks for the KIP. Optional fields and var length encoding support is
> a great
> > > improvement for the protocol.
> > >
> > > >>Optional fields can have any type, except that they cannot be arrays.
> > > Note that the restriction against having tagged arrays is just to
> simplify
> > > serialization.  We can relax this restriction in the future without
> changing
> > > the protocol on the wire.
> > >
> > > Can an Optional field have a struct type which internally contains an
> array
> > > field at any level?
> > >
> > > Thanks,
> > > Satish.
> > >
> > >
> > >
> > > On Tue, Aug 13, 2019 at 11:49 PM David Jacot <dj...@confluent.io>
> wrote:
> > > >
> > > > Hi Colin,
> > > >
> > > > Thank you for the KIP! Things are well explained!. It is huge
> improvement
> > > > for the Kafka protocol. I have few comments on the proposal:
> > > >
> > > > 1. The interleaved tag/length header sounds like a great
> optimisation as it
> > > > would be shorter on average. The downside, as
> > > > you already pointed out, is that it makes the decoding and the specs
> more
> > > > complex. Personally, I would also favour using two
> > > > vaints in this particular case to keep things simple.
> > > >
> > > > 2. As discussed, I wonder if it would make sense to extend to KIP to
> also
> > > > support optional fields in the Record Header. I think
> > > > that it could be interesting to have such capability for common
> fields
> > > > across all the requests or responses (e.g. tracing id).
> > > >
> > > > Regards,
> > > > David
> > > >
> > > >
> > > >
> > > > On Tue, Aug 13, 2019 at 10:00 AM Jason Gustafson <ja...@confluent.io>
> wrote:
> > > >
> > > > > > Right, I was planning on doing exactly that for all the
> auto-generated
> > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s
> probably a
> > > > > better use of time to convert the manual ones to auto gen first
> (with the
> > > > > possible exception of Fetch/Produce, where the ROI may be higher
> for the
> > > > > manual work)
> > > > >
> > > > > Yeah, that makes sense. Maybe we can include the version bump for
> all RPCs
> > > > > in this KIP, but we can implement it lazily as the protocols are
> converted.
> > > > >
> > > > > -Jason
> > > > >
> > > > > On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org>
> wrote:
> > > > >
> > > > > > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > > > > > Hi Colin,
> > > > > > >
> > > > > > > Thanks for the KIP! This is a significant improvement. One of
> my
> > > > > personal
> > > > > > > interests in this proposal is solving the compatibility
> problems we
> > > > > have
> > > > > > > with the internal schemas used to define consumer offsets and
> > > > > transaction
> > > > > > > metadata. Currently we have to guard schema bumps with the
> inter-broker
> > > > > > > protocol format. Once the format is bumped, there is no way to
> > > > > downgrade.
> > > > > > > By fixing this, we can potentially begin using the new schemas
> before
> > > > > the
> > > > > > > IBP is bumped while still allowing downgrade.
> > > > > > >
> > > > > > > There are a surprising number of other situations we have
> encountered
> > > > > > this
> > > > > > > sort of problem. We have hacked around it in special cases by
> allowing
> > > > > > > nullable fields to the end of the schema, but this is not
> really an
> > > > > > > extensible approach. I'm looking forward to having a better
> option.
> > > > > >
> > > > > > Yeah, this problem keeps coming up.
> > > > > >
> > > > > > >
> > > > > > > With that said, I have a couple questions on the proposal:
> > > > > > >
> > > > > > > 1. For each request API, we need one version bump to begin
> support for
> > > > > > > "flexible versions." Until then, we won't have the option of
> using
> > > > > tagged
> > > > > > > fields even if the broker knows how to handle them. Does it
> make sense
> > > > > to
> > > > > > > go ahead and do a universal bump of each request API now so
> that we'll
> > > > > > have
> > > > > > > this option going forward?
> > > > > >
> > > > > > Right, I was planning on doing exactly that for all the
> auto-generated
> > > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s
> probably a
> > > > > > better use of time to convert the manual ones to auto gen first
> (with the
> > > > > > possible exception of Fetch/Produce, where the ROI may be higher
> for the
> > > > > > manual work)
> > > > > >
> > > > > > > 2. The alternating length/tag header encoding lets us save a
> byte in
> > > > > the
> > > > > > > common case. The downside is that it's a bit more complex to
> specify.
> > > > > It
> > > > > > > also has some extra cost if the length exceeds the tag
> substantially.
> > > > > > > Basically we'd have to pad the tag, right? I think I'm
> wondering if we
> > > > > > > should just bite the bullet and use two varints instead.
> > > > > >
> > > > > > That’s a fair point. It would be shorter on average, but worse
> for some
> > > > > > exceptional cases. Also, the decoding would be more complex,
> which might
> > > > > be
> > > > > > a good reason to go for just having two varints. Yeah, let’s
> simplify.
> > > > > >
> > > > > > Regards,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Jason
> > > > > > >
> > > > > > >
> > > > > > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <
> cmccabe@apache.org>
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > I've made some updates to this KIP. Specifically, I wanted
> to avoid
> > > > > > > > including escape bytes in the serialization format, since it
> was too
> > > > > > > > complex. Also, I think this is a good opportunity to slim
> down our
> > > > > > > > variable length fields.
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > >
> > > > > > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio
> wrote:
> > > > > > > > > > Thanks Colin for the KIP. For my own edification why are
> we doing
> > > > > > this
> > > > > > > > > > "Optional fields can have any type, except for an array
> of
> > > > > > > > structures."?
> > > > > > > > > > Why can't we have an array of structures?
> > > > > > > > >
> > > > > > > > > Optional fields are serialized starting with their total
> length.
> > > > > This
> > > > > > > > > is straightforward to calculate for primitive fields like
> INT32,
> > > > > (or
> > > > > > > > > even an array of INT32), but more difficult to calculate
> for an
> > > > > array
> > > > > > > > > of structures. Basically, we'd have to do a two-pass
> serialization
> > > > > > > > > where we first calculate the lengths of everything, and
> then write
> > > > > it
> > > > > > > > > out.
> > > > > > > > >
> > > > > > > > > The nice thing about this KIP is that there's nothing in
> the
> > > > > protocol
> > > > > > > > > stopping us from adding support for this feature in the
> future. We
> > > > > > > > > wouldn't have to really change the protocol at all to add
> support.
> > > > > > But
> > > > > > > > > we'd have to change a lot of serialization code. Given
> almost all
> > > > > of
> > > > > > > > > our use-cases for optional fields are adding an extra
> field here or
> > > > > > > > > there, it seems reasonable not to support it for right now.
> > > > > > > > >
> > > > > > > > > best,
> > > > > > > > > Colin
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > -Jose
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
>

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Colin McCabe <cm...@apache.org>]

On Mon, Aug 26, 2019, at 05:23, Magnus Edenhill wrote:
> Great KIP as always, Colin!
> 
> Some comments:
> 
> >  If the flexible versions are not specified, it is assumed that all
> versions are flexible.
> 
> This is ambiguous, if a protocol-generator is pointed to an older Kafka
> protocol specification
> it can't know if the lack of flexibleVersions field means they're all
> flexible, or flexibleVersions
> is not supported at all.
> I recommend requiring the flexibleVersions field if there are flexible
> versions.

Hi Magnus,

Thanks for taking a look.

The issue that I was trying to avoid here is people creating new files and inadvertently forgetting to set flexibleVersions.  Maybe a better way to do it would be to make flexibleVersions a required field, though.  I will change the KIP to specify that as a requirement.

> 
> 
> > They are serialized in ascending order of their tag.
> 
> I'm not opposed to it, but what's the reasoning for requiring the tag list
> to be ordered?
> And if it is a requirement, make the wording stronger ("required"),
> otherwise remove it.
> 

Good question.  The reason is that I didn't want to create a situation where there were multiple different ways to serialize the same data.  By having a single canonical way of serializing it, we can compare two serialized buffers and know if they represent the same data or different data without deserializing them.  This is very powerful, I think.  For example, we can look at the checksum of two RPCs and know that if they are different, at least something is different in those RPCs.

> 
> > All requests and responses will end with a tagged field buffer.  If there
> are no tagged fields, this will only be a single zero byte.
> 
> Since the tagged fields can (or most likely will) contain information that
> has bearing on the information in the request or response, it'd be better if
> the tagged fields were inserted before the request/response body.
> A streaming protocol parser would otherwise have to perform two passes of
> the req/resp (worst case).

Hmm.  I'm not sure I follow.  Each tagged field will be serialized directly after the struct that contains it.  There's no need for two passes when reading, right?  When you read a struct, some fields come first, and some come later, but that doesn't require two passes.

The rationale for putting the tagged fields after the structure was that we must do this for the request and response header, and it's simpler to be consistent than to do something different in the other cases.

> 
> 
> Addtional comments:
> 
> 1)
> With request/response tags we can now introduce request-level errors,
> allowing the broker
> to inform the client why a request was rejected before closing down the
> connection.
> E.g., an empty response body with tag 0 as the error code and tag 1 as the
> error 

Yeah... I thought about this a bit as well.  We could add a generic response error as a tagged field in the response header.  However, I think the details of this are complex enough that it's probably worth its own follow-on KIP.

> 
> 2)
> The protocol generator should error out if there are duplicate tags for a
> context,
> or if a context has tagged fields but no flexibleVersions, to help catch
> such errors early.

For the first part, the Jackson library takes care of worrying about duplicate tags.  For the second part, I agree, it should be an error if there are tagged fields but no flexibleVersions specified.

> 
> 3)
> A semi-related ask:
> there's currently some non-JSON comments in the protocol spec files,
> typically version history.
> If the information in the comment is important enough to be in the spec
> file, it should be in
> a proper JSON field (e.g., about), which makes sure it ends up in the
> generated protocol docs instead of being filtered out.
> 

Yeah, we could consider reorganizing this a bit.  This seems like a separate issue from tagged fields, though.

best,
Colin

> 
> /Magnus
> 
> 
> Den mån 19 aug. 2019 kl 19:00 skrev Colin McCabe <cm...@apache.org>:
> 
> > Hi Satish,
> >
> > I wasn't originally going to propose supporting the struct type, although
> > perhaps we could consider it.
> >
> > In general, supporting a struct containing an array has the same
> > serialization issues as just supporting the array.
> >
> > Probably, we should just have a two-pass serialization mechanism where we
> > calculate lengths first and then write out bytes.  If we do that, we can
> > avoid having any restrictions on tagged fields vs. regular fields.  I will
> > take a look at how complex this would be.
> >
> > best,
> > Colin
> >
> >
> > On Sun, Aug 18, 2019, at 22:27, Satish Duggana wrote:
> > > Please read struct type as a complex record type in my earlier mail.
> > > The complex type seems to be defined as Schema[1] in the protocol
> > > types.
> > >
> > > 1.
> > >
> > https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java#L27
> > >
> > >
> > > On Mon, Aug 19, 2019 at 9:46 AM Satish Duggana <sa...@gmail.com>
> > wrote:
> > > >
> > > > Sorry! Colin, I may not have been clear in my earlier query about
> > > > optional field type restriction. It is mentioned in one of your
> > > > replies "optional fields are serialized starting with their total
> > > > length". This brings the question of whether optional fields support
> > > > struct types (with or without array values). It seems struct types are
> > > > currently not serialized with total length. I may be missing something
> > > > here.
> > > >
> > > > Thanks,
> > > > Satish.
> > > >
> > > >
> > > > On Wed, Aug 14, 2019 at 8:03 AM Satish Duggana <
> > satish.duggana@gmail.com> wrote:
> > > > >
> > > > > Hi Colin,
> > > > > Thanks for the KIP. Optional fields and var length encoding support
> > is a great
> > > > > improvement for the protocol.
> > > > >
> > > > > >>Optional fields can have any type, except that they cannot be
> > arrays.
> > > > > Note that the restriction against having tagged arrays is just to
> > simplify
> > > > > serialization.  We can relax this restriction in the future without
> > changing
> > > > > the protocol on the wire.
> > > > >
> > > > > Can an Optional field have a struct type which internally contains
> > an array
> > > > > field at any level?
> > > > >
> > > > > Thanks,
> > > > > Satish.
> > > > >
> > > > >
> > > > >
> > > > > On Tue, Aug 13, 2019 at 11:49 PM David Jacot <dj...@confluent.io>
> > wrote:
> > > > > >
> > > > > > Hi Colin,
> > > > > >
> > > > > > Thank you for the KIP! Things are well explained!. It is huge
> > improvement
> > > > > > for the Kafka protocol. I have few comments on the proposal:
> > > > > >
> > > > > > 1. The interleaved tag/length header sounds like a great
> > optimisation as it
> > > > > > would be shorter on average. The downside, as
> > > > > > you already pointed out, is that it makes the decoding and the
> > specs more
> > > > > > complex. Personally, I would also favour using two
> > > > > > vaints in this particular case to keep things simple.
> > > > > >
> > > > > > 2. As discussed, I wonder if it would make sense to extend to KIP
> > to also
> > > > > > support optional fields in the Record Header. I think
> > > > > > that it could be interesting to have such capability for common
> > fields
> > > > > > across all the requests or responses (e.g. tracing id).
> > > > > >
> > > > > > Regards,
> > > > > > David
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Tue, Aug 13, 2019 at 10:00 AM Jason Gustafson <
> > jason@confluent.io> wrote:
> > > > > >
> > > > > > > > Right, I was planning on doing exactly that for all the
> > auto-generated
> > > > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s
> > probably a
> > > > > > > better use of time to convert the manual ones to auto gen first
> > (with the
> > > > > > > possible exception of Fetch/Produce, where the ROI may be higher
> > for the
> > > > > > > manual work)
> > > > > > >
> > > > > > > Yeah, that makes sense. Maybe we can include the version bump
> > for all RPCs
> > > > > > > in this KIP, but we can implement it lazily as the protocols are
> > converted.
> > > > > > >
> > > > > > > -Jason
> > > > > > >
> > > > > > > On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org>
> > wrote:
> > > > > > >
> > > > > > > > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > > > > > > > Hi Colin,
> > > > > > > > >
> > > > > > > > > Thanks for the KIP! This is a significant improvement. One
> > of my
> > > > > > > personal
> > > > > > > > > interests in this proposal is solving the compatibility
> > problems we
> > > > > > > have
> > > > > > > > > with the internal schemas used to define consumer offsets and
> > > > > > > transaction
> > > > > > > > > metadata. Currently we have to guard schema bumps with the
> > inter-broker
> > > > > > > > > protocol format. Once the format is bumped, there is no way
> > to
> > > > > > > downgrade.
> > > > > > > > > By fixing this, we can potentially begin using the new
> > schemas before
> > > > > > > the
> > > > > > > > > IBP is bumped while still allowing downgrade.
> > > > > > > > >
> > > > > > > > > There are a surprising number of other situations we have
> > encountered
> > > > > > > > this
> > > > > > > > > sort of problem. We have hacked around it in special cases
> > by allowing
> > > > > > > > > nullable fields to the end of the schema, but this is not
> > really an
> > > > > > > > > extensible approach. I'm looking forward to having a better
> > option.
> > > > > > > >
> > > > > > > > Yeah, this problem keeps coming up.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > With that said, I have a couple questions on the proposal:
> > > > > > > > >
> > > > > > > > > 1. For each request API, we need one version bump to begin
> > support for
> > > > > > > > > "flexible versions." Until then, we won't have the option of
> > using
> > > > > > > tagged
> > > > > > > > > fields even if the broker knows how to handle them. Does it
> > make sense
> > > > > > > to
> > > > > > > > > go ahead and do a universal bump of each request API now so
> > that we'll
> > > > > > > > have
> > > > > > > > > this option going forward?
> > > > > > > >
> > > > > > > > Right, I was planning on doing exactly that for all the
> > auto-generated
> > > > > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s
> > probably a
> > > > > > > > better use of time to convert the manual ones to auto gen
> > first (with the
> > > > > > > > possible exception of Fetch/Produce, where the ROI may be
> > higher for the
> > > > > > > > manual work)
> > > > > > > >
> > > > > > > > > 2. The alternating length/tag header encoding lets us save a
> > byte in
> > > > > > > the
> > > > > > > > > common case. The downside is that it's a bit more complex to
> > specify.
> > > > > > > It
> > > > > > > > > also has some extra cost if the length exceeds the tag
> > substantially.
> > > > > > > > > Basically we'd have to pad the tag, right? I think I'm
> > wondering if we
> > > > > > > > > should just bite the bullet and use two varints instead.
> > > > > > > >
> > > > > > > > That’s a fair point. It would be shorter on average, but worse
> > for some
> > > > > > > > exceptional cases. Also, the decoding would be more complex,
> > which might
> > > > > > > be
> > > > > > > > a good reason to go for just having two varints. Yeah, let’s
> > simplify.
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Jason
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <
> > cmccabe@apache.org>
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi all,
> > > > > > > > > >
> > > > > > > > > > I've made some updates to this KIP. Specifically, I wanted
> > to avoid
> > > > > > > > > > including escape bytes in the serialization format, since
> > it was too
> > > > > > > > > > complex. Also, I think this is a good opportunity to slim
> > down our
> > > > > > > > > > variable length fields.
> > > > > > > > > >
> > > > > > > > > > best,
> > > > > > > > > > Colin
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > > > > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia
> > Sancio wrote:
> > > > > > > > > > > > Thanks Colin for the KIP. For my own edification why
> > are we doing
> > > > > > > > this
> > > > > > > > > > > > "Optional fields can have any type, except for an
> > array of
> > > > > > > > > > structures."?
> > > > > > > > > > > > Why can't we have an array of structures?
> > > > > > > > > > >
> > > > > > > > > > > Optional fields are serialized starting with their total
> > length.
> > > > > > > This
> > > > > > > > > > > is straightforward to calculate for primitive fields
> > like INT32,
> > > > > > > (or
> > > > > > > > > > > even an array of INT32), but more difficult to calculate
> > for an
> > > > > > > array
> > > > > > > > > > > of structures. Basically, we'd have to do a two-pass
> > serialization
> > > > > > > > > > > where we first calculate the lengths of everything, and
> > then write
> > > > > > > it
> > > > > > > > > > > out.
> > > > > > > > > > >
> > > > > > > > > > > The nice thing about this KIP is that there's nothing in
> > the
> > > > > > > protocol
> > > > > > > > > > > stopping us from adding support for this feature in the
> > future. We
> > > > > > > > > > > wouldn't have to really change the protocol at all to
> > add support.
> > > > > > > > But
> > > > > > > > > > > we'd have to change a lot of serialization code. Given
> > almost all
> > > > > > > of
> > > > > > > > > > > our use-cases for optional fields are adding an extra
> > field here or
> > > > > > > > > > > there, it seems reasonable not to support it for right
> > now.
> > > > > > > > > > >
> > > > > > > > > > > best,
> > > > > > > > > > > Colin
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > --
> > > > > > > > > > > > -Jose
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > >
> >
>

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Magnus Edenhill <ma...@edenhill.se>]

Great KIP as always, Colin!

Some comments:

>  If the flexible versions are not specified, it is assumed that all
versions are flexible.

This is ambiguous, if a protocol-generator is pointed to an older Kafka
protocol specification
it can't know if the lack of flexibleVersions field means they're all
flexible, or flexibleVersions
is not supported at all.
I recommend requiring the flexibleVersions field if there are flexible
versions.


> They are serialized in ascending order of their tag.

I'm not opposed to it, but what's the reasoning for requiring the tag list
to be ordered?
And if it is a requirement, make the wording stronger ("required"),
otherwise remove it.


> All requests and responses will end with a tagged field buffer.  If there
are no tagged fields, this will only be a single zero byte.

Since the tagged fields can (or most likely will) contain information that
has bearing on the information in the request or response, it'd be better if
the tagged fields were inserted before the request/response body.
A streaming protocol parser would otherwise have to perform two passes of
the req/resp (worst case).


Addtional comments:

1)
With request/response tags we can now introduce request-level errors,
allowing the broker
to inform the client why a request was rejected before closing down the
connection.
E.g., an empty response body with tag 0 as the error code and tag 1 as the
error string.

2)
The protocol generator should error out if there are duplicate tags for a
context,
or if a context has tagged fields but no flexibleVersions, to help catch
such errors early.


3)
A semi-related ask:
there's currently some non-JSON comments in the protocol spec files,
typically version history.
If the information in the comment is important enough to be in the spec
file, it should be in
a proper JSON field (e.g., about), which makes sure it ends up in the
generated protocol docs instead of being filtered out.


/Magnus


Den mån 19 aug. 2019 kl 19:00 skrev Colin McCabe <cm...@apache.org>:

> Hi Satish,
>
> I wasn't originally going to propose supporting the struct type, although
> perhaps we could consider it.
>
> In general, supporting a struct containing an array has the same
> serialization issues as just supporting the array.
>
> Probably, we should just have a two-pass serialization mechanism where we
> calculate lengths first and then write out bytes.  If we do that, we can
> avoid having any restrictions on tagged fields vs. regular fields.  I will
> take a look at how complex this would be.
>
> best,
> Colin
>
>
> On Sun, Aug 18, 2019, at 22:27, Satish Duggana wrote:
> > Please read struct type as a complex record type in my earlier mail.
> > The complex type seems to be defined as Schema[1] in the protocol
> > types.
> >
> > 1.
> >
> https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java#L27
> >
> >
> > On Mon, Aug 19, 2019 at 9:46 AM Satish Duggana <sa...@gmail.com>
> wrote:
> > >
> > > Sorry! Colin, I may not have been clear in my earlier query about
> > > optional field type restriction. It is mentioned in one of your
> > > replies "optional fields are serialized starting with their total
> > > length". This brings the question of whether optional fields support
> > > struct types (with or without array values). It seems struct types are
> > > currently not serialized with total length. I may be missing something
> > > here.
> > >
> > > Thanks,
> > > Satish.
> > >
> > >
> > > On Wed, Aug 14, 2019 at 8:03 AM Satish Duggana <
> satish.duggana@gmail.com> wrote:
> > > >
> > > > Hi Colin,
> > > > Thanks for the KIP. Optional fields and var length encoding support
> is a great
> > > > improvement for the protocol.
> > > >
> > > > >>Optional fields can have any type, except that they cannot be
> arrays.
> > > > Note that the restriction against having tagged arrays is just to
> simplify
> > > > serialization.  We can relax this restriction in the future without
> changing
> > > > the protocol on the wire.
> > > >
> > > > Can an Optional field have a struct type which internally contains
> an array
> > > > field at any level?
> > > >
> > > > Thanks,
> > > > Satish.
> > > >
> > > >
> > > >
> > > > On Tue, Aug 13, 2019 at 11:49 PM David Jacot <dj...@confluent.io>
> wrote:
> > > > >
> > > > > Hi Colin,
> > > > >
> > > > > Thank you for the KIP! Things are well explained!. It is huge
> improvement
> > > > > for the Kafka protocol. I have few comments on the proposal:
> > > > >
> > > > > 1. The interleaved tag/length header sounds like a great
> optimisation as it
> > > > > would be shorter on average. The downside, as
> > > > > you already pointed out, is that it makes the decoding and the
> specs more
> > > > > complex. Personally, I would also favour using two
> > > > > vaints in this particular case to keep things simple.
> > > > >
> > > > > 2. As discussed, I wonder if it would make sense to extend to KIP
> to also
> > > > > support optional fields in the Record Header. I think
> > > > > that it could be interesting to have such capability for common
> fields
> > > > > across all the requests or responses (e.g. tracing id).
> > > > >
> > > > > Regards,
> > > > > David
> > > > >
> > > > >
> > > > >
> > > > > On Tue, Aug 13, 2019 at 10:00 AM Jason Gustafson <
> jason@confluent.io> wrote:
> > > > >
> > > > > > > Right, I was planning on doing exactly that for all the
> auto-generated
> > > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s
> probably a
> > > > > > better use of time to convert the manual ones to auto gen first
> (with the
> > > > > > possible exception of Fetch/Produce, where the ROI may be higher
> for the
> > > > > > manual work)
> > > > > >
> > > > > > Yeah, that makes sense. Maybe we can include the version bump
> for all RPCs
> > > > > > in this KIP, but we can implement it lazily as the protocols are
> converted.
> > > > > >
> > > > > > -Jason
> > > > > >
> > > > > > On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org>
> wrote:
> > > > > >
> > > > > > > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > > > > > > Hi Colin,
> > > > > > > >
> > > > > > > > Thanks for the KIP! This is a significant improvement. One
> of my
> > > > > > personal
> > > > > > > > interests in this proposal is solving the compatibility
> problems we
> > > > > > have
> > > > > > > > with the internal schemas used to define consumer offsets and
> > > > > > transaction
> > > > > > > > metadata. Currently we have to guard schema bumps with the
> inter-broker
> > > > > > > > protocol format. Once the format is bumped, there is no way
> to
> > > > > > downgrade.
> > > > > > > > By fixing this, we can potentially begin using the new
> schemas before
> > > > > > the
> > > > > > > > IBP is bumped while still allowing downgrade.
> > > > > > > >
> > > > > > > > There are a surprising number of other situations we have
> encountered
> > > > > > > this
> > > > > > > > sort of problem. We have hacked around it in special cases
> by allowing
> > > > > > > > nullable fields to the end of the schema, but this is not
> really an
> > > > > > > > extensible approach. I'm looking forward to having a better
> option.
> > > > > > >
> > > > > > > Yeah, this problem keeps coming up.
> > > > > > >
> > > > > > > >
> > > > > > > > With that said, I have a couple questions on the proposal:
> > > > > > > >
> > > > > > > > 1. For each request API, we need one version bump to begin
> support for
> > > > > > > > "flexible versions." Until then, we won't have the option of
> using
> > > > > > tagged
> > > > > > > > fields even if the broker knows how to handle them. Does it
> make sense
> > > > > > to
> > > > > > > > go ahead and do a universal bump of each request API now so
> that we'll
> > > > > > > have
> > > > > > > > this option going forward?
> > > > > > >
> > > > > > > Right, I was planning on doing exactly that for all the
> auto-generated
> > > > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s
> probably a
> > > > > > > better use of time to convert the manual ones to auto gen
> first (with the
> > > > > > > possible exception of Fetch/Produce, where the ROI may be
> higher for the
> > > > > > > manual work)
> > > > > > >
> > > > > > > > 2. The alternating length/tag header encoding lets us save a
> byte in
> > > > > > the
> > > > > > > > common case. The downside is that it's a bit more complex to
> specify.
> > > > > > It
> > > > > > > > also has some extra cost if the length exceeds the tag
> substantially.
> > > > > > > > Basically we'd have to pad the tag, right? I think I'm
> wondering if we
> > > > > > > > should just bite the bullet and use two varints instead.
> > > > > > >
> > > > > > > That’s a fair point. It would be shorter on average, but worse
> for some
> > > > > > > exceptional cases. Also, the decoding would be more complex,
> which might
> > > > > > be
> > > > > > > a good reason to go for just having two varints. Yeah, let’s
> simplify.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Colin
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Jason
> > > > > > > >
> > > > > > > >
> > > > > > > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <
> cmccabe@apache.org>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi all,
> > > > > > > > >
> > > > > > > > > I've made some updates to this KIP. Specifically, I wanted
> to avoid
> > > > > > > > > including escape bytes in the serialization format, since
> it was too
> > > > > > > > > complex. Also, I think this is a good opportunity to slim
> down our
> > > > > > > > > variable length fields.
> > > > > > > > >
> > > > > > > > > best,
> > > > > > > > > Colin
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > > > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia
> Sancio wrote:
> > > > > > > > > > > Thanks Colin for the KIP. For my own edification why
> are we doing
> > > > > > > this
> > > > > > > > > > > "Optional fields can have any type, except for an
> array of
> > > > > > > > > structures."?
> > > > > > > > > > > Why can't we have an array of structures?
> > > > > > > > > >
> > > > > > > > > > Optional fields are serialized starting with their total
> length.
> > > > > > This
> > > > > > > > > > is straightforward to calculate for primitive fields
> like INT32,
> > > > > > (or
> > > > > > > > > > even an array of INT32), but more difficult to calculate
> for an
> > > > > > array
> > > > > > > > > > of structures. Basically, we'd have to do a two-pass
> serialization
> > > > > > > > > > where we first calculate the lengths of everything, and
> then write
> > > > > > it
> > > > > > > > > > out.
> > > > > > > > > >
> > > > > > > > > > The nice thing about this KIP is that there's nothing in
> the
> > > > > > protocol
> > > > > > > > > > stopping us from adding support for this feature in the
> future. We
> > > > > > > > > > wouldn't have to really change the protocol at all to
> add support.
> > > > > > > But
> > > > > > > > > > we'd have to change a lot of serialization code. Given
> almost all
> > > > > > of
> > > > > > > > > > our use-cases for optional fields are adding an extra
> field here or
> > > > > > > > > > there, it seems reasonable not to support it for right
> now.
> > > > > > > > > >
> > > > > > > > > > best,
> > > > > > > > > > Colin
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > -Jose
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> >
>

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Colin McCabe <cm...@apache.org>]

Hi Satish,

I wasn't originally going to propose supporting the struct type, although perhaps we could consider it.

In general, supporting a struct containing an array has the same serialization issues as just supporting the array.

Probably, we should just have a two-pass serialization mechanism where we calculate lengths first and then write out bytes.  If we do that, we can avoid having any restrictions on tagged fields vs. regular fields.  I will take a look at how complex this would be.

best,
Colin


On Sun, Aug 18, 2019, at 22:27, Satish Duggana wrote:
> Please read struct type as a complex record type in my earlier mail.
> The complex type seems to be defined as Schema[1] in the protocol
> types.
> 
> 1. 
> https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java#L27
> 
> 
> On Mon, Aug 19, 2019 at 9:46 AM Satish Duggana <sa...@gmail.com> wrote:
> >
> > Sorry! Colin, I may not have been clear in my earlier query about
> > optional field type restriction. It is mentioned in one of your
> > replies "optional fields are serialized starting with their total
> > length". This brings the question of whether optional fields support
> > struct types (with or without array values). It seems struct types are
> > currently not serialized with total length. I may be missing something
> > here.
> >
> > Thanks,
> > Satish.
> >
> >
> > On Wed, Aug 14, 2019 at 8:03 AM Satish Duggana <sa...@gmail.com> wrote:
> > >
> > > Hi Colin,
> > > Thanks for the KIP. Optional fields and var length encoding support is a great
> > > improvement for the protocol.
> > >
> > > >>Optional fields can have any type, except that they cannot be arrays.
> > > Note that the restriction against having tagged arrays is just to simplify
> > > serialization.  We can relax this restriction in the future without changing
> > > the protocol on the wire.
> > >
> > > Can an Optional field have a struct type which internally contains an array
> > > field at any level?
> > >
> > > Thanks,
> > > Satish.
> > >
> > >
> > >
> > > On Tue, Aug 13, 2019 at 11:49 PM David Jacot <dj...@confluent.io> wrote:
> > > >
> > > > Hi Colin,
> > > >
> > > > Thank you for the KIP! Things are well explained!. It is huge improvement
> > > > for the Kafka protocol. I have few comments on the proposal:
> > > >
> > > > 1. The interleaved tag/length header sounds like a great optimisation as it
> > > > would be shorter on average. The downside, as
> > > > you already pointed out, is that it makes the decoding and the specs more
> > > > complex. Personally, I would also favour using two
> > > > vaints in this particular case to keep things simple.
> > > >
> > > > 2. As discussed, I wonder if it would make sense to extend to KIP to also
> > > > support optional fields in the Record Header. I think
> > > > that it could be interesting to have such capability for common fields
> > > > across all the requests or responses (e.g. tracing id).
> > > >
> > > > Regards,
> > > > David
> > > >
> > > >
> > > >
> > > > On Tue, Aug 13, 2019 at 10:00 AM Jason Gustafson <ja...@confluent.io> wrote:
> > > >
> > > > > > Right, I was planning on doing exactly that for all the auto-generated
> > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > > > > better use of time to convert the manual ones to auto gen first (with the
> > > > > possible exception of Fetch/Produce, where the ROI may be higher for the
> > > > > manual work)
> > > > >
> > > > > Yeah, that makes sense. Maybe we can include the version bump for all RPCs
> > > > > in this KIP, but we can implement it lazily as the protocols are converted.
> > > > >
> > > > > -Jason
> > > > >
> > > > > On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org> wrote:
> > > > >
> > > > > > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > > > > > Hi Colin,
> > > > > > >
> > > > > > > Thanks for the KIP! This is a significant improvement. One of my
> > > > > personal
> > > > > > > interests in this proposal is solving the compatibility problems we
> > > > > have
> > > > > > > with the internal schemas used to define consumer offsets and
> > > > > transaction
> > > > > > > metadata. Currently we have to guard schema bumps with the inter-broker
> > > > > > > protocol format. Once the format is bumped, there is no way to
> > > > > downgrade.
> > > > > > > By fixing this, we can potentially begin using the new schemas before
> > > > > the
> > > > > > > IBP is bumped while still allowing downgrade.
> > > > > > >
> > > > > > > There are a surprising number of other situations we have encountered
> > > > > > this
> > > > > > > sort of problem. We have hacked around it in special cases by allowing
> > > > > > > nullable fields to the end of the schema, but this is not really an
> > > > > > > extensible approach. I'm looking forward to having a better option.
> > > > > >
> > > > > > Yeah, this problem keeps coming up.
> > > > > >
> > > > > > >
> > > > > > > With that said, I have a couple questions on the proposal:
> > > > > > >
> > > > > > > 1. For each request API, we need one version bump to begin support for
> > > > > > > "flexible versions." Until then, we won't have the option of using
> > > > > tagged
> > > > > > > fields even if the broker knows how to handle them. Does it make sense
> > > > > to
> > > > > > > go ahead and do a universal bump of each request API now so that we'll
> > > > > > have
> > > > > > > this option going forward?
> > > > > >
> > > > > > Right, I was planning on doing exactly that for all the auto-generated
> > > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > > > > > better use of time to convert the manual ones to auto gen first (with the
> > > > > > possible exception of Fetch/Produce, where the ROI may be higher for the
> > > > > > manual work)
> > > > > >
> > > > > > > 2. The alternating length/tag header encoding lets us save a byte in
> > > > > the
> > > > > > > common case. The downside is that it's a bit more complex to specify.
> > > > > It
> > > > > > > also has some extra cost if the length exceeds the tag substantially.
> > > > > > > Basically we'd have to pad the tag, right? I think I'm wondering if we
> > > > > > > should just bite the bullet and use two varints instead.
> > > > > >
> > > > > > That’s a fair point. It would be shorter on average, but worse for some
> > > > > > exceptional cases. Also, the decoding would be more complex, which might
> > > > > be
> > > > > > a good reason to go for just having two varints. Yeah, let’s simplify.
> > > > > >
> > > > > > Regards,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Jason
> > > > > > >
> > > > > > >
> > > > > > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <cm...@apache.org>
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > I've made some updates to this KIP. Specifically, I wanted to avoid
> > > > > > > > including escape bytes in the serialization format, since it was too
> > > > > > > > complex. Also, I think this is a good opportunity to slim down our
> > > > > > > > variable length fields.
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > >
> > > > > > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > > > > > > > > > Thanks Colin for the KIP. For my own edification why are we doing
> > > > > > this
> > > > > > > > > > "Optional fields can have any type, except for an array of
> > > > > > > > structures."?
> > > > > > > > > > Why can't we have an array of structures?
> > > > > > > > >
> > > > > > > > > Optional fields are serialized starting with their total length.
> > > > > This
> > > > > > > > > is straightforward to calculate for primitive fields like INT32,
> > > > > (or
> > > > > > > > > even an array of INT32), but more difficult to calculate for an
> > > > > array
> > > > > > > > > of structures. Basically, we'd have to do a two-pass serialization
> > > > > > > > > where we first calculate the lengths of everything, and then write
> > > > > it
> > > > > > > > > out.
> > > > > > > > >
> > > > > > > > > The nice thing about this KIP is that there's nothing in the
> > > > > protocol
> > > > > > > > > stopping us from adding support for this feature in the future. We
> > > > > > > > > wouldn't have to really change the protocol at all to add support.
> > > > > > But
> > > > > > > > > we'd have to change a lot of serialization code. Given almost all
> > > > > of
> > > > > > > > > our use-cases for optional fields are adding an extra field here or
> > > > > > > > > there, it seems reasonable not to support it for right now.
> > > > > > > > >
> > > > > > > > > best,
> > > > > > > > > Colin
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > -Jose
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
>

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Satish Duggana <sa...@gmail.com>]

Please read struct type as a complex record type in my earlier mail.
The complex type seems to be defined as Schema[1] in the protocol
types.

1. https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java#L27


On Mon, Aug 19, 2019 at 9:46 AM Satish Duggana <sa...@gmail.com> wrote:
>
> Sorry! Colin, I may not have been clear in my earlier query about
> optional field type restriction. It is mentioned in one of your
> replies "optional fields are serialized starting with their total
> length". This brings the question of whether optional fields support
> struct types (with or without array values). It seems struct types are
> currently not serialized with total length. I may be missing something
> here.
>
> Thanks,
> Satish.
>
>
> On Wed, Aug 14, 2019 at 8:03 AM Satish Duggana <sa...@gmail.com> wrote:
> >
> > Hi Colin,
> > Thanks for the KIP. Optional fields and var length encoding support is a great
> > improvement for the protocol.
> >
> > >>Optional fields can have any type, except that they cannot be arrays.
> > Note that the restriction against having tagged arrays is just to simplify
> > serialization.  We can relax this restriction in the future without changing
> > the protocol on the wire.
> >
> > Can an Optional field have a struct type which internally contains an array
> > field at any level?
> >
> > Thanks,
> > Satish.
> >
> >
> >
> > On Tue, Aug 13, 2019 at 11:49 PM David Jacot <dj...@confluent.io> wrote:
> > >
> > > Hi Colin,
> > >
> > > Thank you for the KIP! Things are well explained!. It is huge improvement
> > > for the Kafka protocol. I have few comments on the proposal:
> > >
> > > 1. The interleaved tag/length header sounds like a great optimisation as it
> > > would be shorter on average. The downside, as
> > > you already pointed out, is that it makes the decoding and the specs more
> > > complex. Personally, I would also favour using two
> > > vaints in this particular case to keep things simple.
> > >
> > > 2. As discussed, I wonder if it would make sense to extend to KIP to also
> > > support optional fields in the Record Header. I think
> > > that it could be interesting to have such capability for common fields
> > > across all the requests or responses (e.g. tracing id).
> > >
> > > Regards,
> > > David
> > >
> > >
> > >
> > > On Tue, Aug 13, 2019 at 10:00 AM Jason Gustafson <ja...@confluent.io> wrote:
> > >
> > > > > Right, I was planning on doing exactly that for all the auto-generated
> > > > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > > > better use of time to convert the manual ones to auto gen first (with the
> > > > possible exception of Fetch/Produce, where the ROI may be higher for the
> > > > manual work)
> > > >
> > > > Yeah, that makes sense. Maybe we can include the version bump for all RPCs
> > > > in this KIP, but we can implement it lazily as the protocols are converted.
> > > >
> > > > -Jason
> > > >
> > > > On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org> wrote:
> > > >
> > > > > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > > > > Hi Colin,
> > > > > >
> > > > > > Thanks for the KIP! This is a significant improvement. One of my
> > > > personal
> > > > > > interests in this proposal is solving the compatibility problems we
> > > > have
> > > > > > with the internal schemas used to define consumer offsets and
> > > > transaction
> > > > > > metadata. Currently we have to guard schema bumps with the inter-broker
> > > > > > protocol format. Once the format is bumped, there is no way to
> > > > downgrade.
> > > > > > By fixing this, we can potentially begin using the new schemas before
> > > > the
> > > > > > IBP is bumped while still allowing downgrade.
> > > > > >
> > > > > > There are a surprising number of other situations we have encountered
> > > > > this
> > > > > > sort of problem. We have hacked around it in special cases by allowing
> > > > > > nullable fields to the end of the schema, but this is not really an
> > > > > > extensible approach. I'm looking forward to having a better option.
> > > > >
> > > > > Yeah, this problem keeps coming up.
> > > > >
> > > > > >
> > > > > > With that said, I have a couple questions on the proposal:
> > > > > >
> > > > > > 1. For each request API, we need one version bump to begin support for
> > > > > > "flexible versions." Until then, we won't have the option of using
> > > > tagged
> > > > > > fields even if the broker knows how to handle them. Does it make sense
> > > > to
> > > > > > go ahead and do a universal bump of each request API now so that we'll
> > > > > have
> > > > > > this option going forward?
> > > > >
> > > > > Right, I was planning on doing exactly that for all the auto-generated
> > > > > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > > > > better use of time to convert the manual ones to auto gen first (with the
> > > > > possible exception of Fetch/Produce, where the ROI may be higher for the
> > > > > manual work)
> > > > >
> > > > > > 2. The alternating length/tag header encoding lets us save a byte in
> > > > the
> > > > > > common case. The downside is that it's a bit more complex to specify.
> > > > It
> > > > > > also has some extra cost if the length exceeds the tag substantially.
> > > > > > Basically we'd have to pad the tag, right? I think I'm wondering if we
> > > > > > should just bite the bullet and use two varints instead.
> > > > >
> > > > > That’s a fair point. It would be shorter on average, but worse for some
> > > > > exceptional cases. Also, the decoding would be more complex, which might
> > > > be
> > > > > a good reason to go for just having two varints. Yeah, let’s simplify.
> > > > >
> > > > > Regards,
> > > > > Colin
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Jason
> > > > > >
> > > > > >
> > > > > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <cm...@apache.org>
> > > > wrote:
> > > > > >
> > > > > > > Hi all,
> > > > > > >
> > > > > > > I've made some updates to this KIP. Specifically, I wanted to avoid
> > > > > > > including escape bytes in the serialization format, since it was too
> > > > > > > complex. Also, I think this is a good opportunity to slim down our
> > > > > > > variable length fields.
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > > >
> > > > > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > > > > > > > > Thanks Colin for the KIP. For my own edification why are we doing
> > > > > this
> > > > > > > > > "Optional fields can have any type, except for an array of
> > > > > > > structures."?
> > > > > > > > > Why can't we have an array of structures?
> > > > > > > >
> > > > > > > > Optional fields are serialized starting with their total length.
> > > > This
> > > > > > > > is straightforward to calculate for primitive fields like INT32,
> > > > (or
> > > > > > > > even an array of INT32), but more difficult to calculate for an
> > > > array
> > > > > > > > of structures. Basically, we'd have to do a two-pass serialization
> > > > > > > > where we first calculate the lengths of everything, and then write
> > > > it
> > > > > > > > out.
> > > > > > > >
> > > > > > > > The nice thing about this KIP is that there's nothing in the
> > > > protocol
> > > > > > > > stopping us from adding support for this feature in the future. We
> > > > > > > > wouldn't have to really change the protocol at all to add support.
> > > > > But
> > > > > > > > we'd have to change a lot of serialization code. Given almost all
> > > > of
> > > > > > > > our use-cases for optional fields are adding an extra field here or
> > > > > > > > there, it seems reasonable not to support it for right now.
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > -Jose
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Satish Duggana <sa...@gmail.com>]

Sorry! Colin, I may not have been clear in my earlier query about
optional field type restriction. It is mentioned in one of your
replies "optional fields are serialized starting with their total
length". This brings the question of whether optional fields support
struct types (with or without array values). It seems struct types are
currently not serialized with total length. I may be missing something
here.

Thanks,
Satish.


On Wed, Aug 14, 2019 at 8:03 AM Satish Duggana <sa...@gmail.com> wrote:
>
> Hi Colin,
> Thanks for the KIP. Optional fields and var length encoding support is a great
> improvement for the protocol.
>
> >>Optional fields can have any type, except that they cannot be arrays.
> Note that the restriction against having tagged arrays is just to simplify
> serialization.  We can relax this restriction in the future without changing
> the protocol on the wire.
>
> Can an Optional field have a struct type which internally contains an array
> field at any level?
>
> Thanks,
> Satish.
>
>
>
> On Tue, Aug 13, 2019 at 11:49 PM David Jacot <dj...@confluent.io> wrote:
> >
> > Hi Colin,
> >
> > Thank you for the KIP! Things are well explained!. It is huge improvement
> > for the Kafka protocol. I have few comments on the proposal:
> >
> > 1. The interleaved tag/length header sounds like a great optimisation as it
> > would be shorter on average. The downside, as
> > you already pointed out, is that it makes the decoding and the specs more
> > complex. Personally, I would also favour using two
> > vaints in this particular case to keep things simple.
> >
> > 2. As discussed, I wonder if it would make sense to extend to KIP to also
> > support optional fields in the Record Header. I think
> > that it could be interesting to have such capability for common fields
> > across all the requests or responses (e.g. tracing id).
> >
> > Regards,
> > David
> >
> >
> >
> > On Tue, Aug 13, 2019 at 10:00 AM Jason Gustafson <ja...@confluent.io> wrote:
> >
> > > > Right, I was planning on doing exactly that for all the auto-generated
> > > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > > better use of time to convert the manual ones to auto gen first (with the
> > > possible exception of Fetch/Produce, where the ROI may be higher for the
> > > manual work)
> > >
> > > Yeah, that makes sense. Maybe we can include the version bump for all RPCs
> > > in this KIP, but we can implement it lazily as the protocols are converted.
> > >
> > > -Jason
> > >
> > > On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org> wrote:
> > >
> > > > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > > > Hi Colin,
> > > > >
> > > > > Thanks for the KIP! This is a significant improvement. One of my
> > > personal
> > > > > interests in this proposal is solving the compatibility problems we
> > > have
> > > > > with the internal schemas used to define consumer offsets and
> > > transaction
> > > > > metadata. Currently we have to guard schema bumps with the inter-broker
> > > > > protocol format. Once the format is bumped, there is no way to
> > > downgrade.
> > > > > By fixing this, we can potentially begin using the new schemas before
> > > the
> > > > > IBP is bumped while still allowing downgrade.
> > > > >
> > > > > There are a surprising number of other situations we have encountered
> > > > this
> > > > > sort of problem. We have hacked around it in special cases by allowing
> > > > > nullable fields to the end of the schema, but this is not really an
> > > > > extensible approach. I'm looking forward to having a better option.
> > > >
> > > > Yeah, this problem keeps coming up.
> > > >
> > > > >
> > > > > With that said, I have a couple questions on the proposal:
> > > > >
> > > > > 1. For each request API, we need one version bump to begin support for
> > > > > "flexible versions." Until then, we won't have the option of using
> > > tagged
> > > > > fields even if the broker knows how to handle them. Does it make sense
> > > to
> > > > > go ahead and do a universal bump of each request API now so that we'll
> > > > have
> > > > > this option going forward?
> > > >
> > > > Right, I was planning on doing exactly that for all the auto-generated
> > > > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > > > better use of time to convert the manual ones to auto gen first (with the
> > > > possible exception of Fetch/Produce, where the ROI may be higher for the
> > > > manual work)
> > > >
> > > > > 2. The alternating length/tag header encoding lets us save a byte in
> > > the
> > > > > common case. The downside is that it's a bit more complex to specify.
> > > It
> > > > > also has some extra cost if the length exceeds the tag substantially.
> > > > > Basically we'd have to pad the tag, right? I think I'm wondering if we
> > > > > should just bite the bullet and use two varints instead.
> > > >
> > > > That’s a fair point. It would be shorter on average, but worse for some
> > > > exceptional cases. Also, the decoding would be more complex, which might
> > > be
> > > > a good reason to go for just having two varints. Yeah, let’s simplify.
> > > >
> > > > Regards,
> > > > Colin
> > > >
> > > > >
> > > > > Thanks,
> > > > > Jason
> > > > >
> > > > >
> > > > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <cm...@apache.org>
> > > wrote:
> > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > I've made some updates to this KIP. Specifically, I wanted to avoid
> > > > > > including escape bytes in the serialization format, since it was too
> > > > > > complex. Also, I think this is a good opportunity to slim down our
> > > > > > variable length fields.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > >
> > > > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > > > > > > > Thanks Colin for the KIP. For my own edification why are we doing
> > > > this
> > > > > > > > "Optional fields can have any type, except for an array of
> > > > > > structures."?
> > > > > > > > Why can't we have an array of structures?
> > > > > > >
> > > > > > > Optional fields are serialized starting with their total length.
> > > This
> > > > > > > is straightforward to calculate for primitive fields like INT32,
> > > (or
> > > > > > > even an array of INT32), but more difficult to calculate for an
> > > array
> > > > > > > of structures. Basically, we'd have to do a two-pass serialization
> > > > > > > where we first calculate the lengths of everything, and then write
> > > it
> > > > > > > out.
> > > > > > >
> > > > > > > The nice thing about this KIP is that there's nothing in the
> > > protocol
> > > > > > > stopping us from adding support for this feature in the future. We
> > > > > > > wouldn't have to really change the protocol at all to add support.
> > > > But
> > > > > > > we'd have to change a lot of serialization code. Given almost all
> > > of
> > > > > > > our use-cases for optional fields are adding an extra field here or
> > > > > > > there, it seems reasonable not to support it for right now.
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > -Jose
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Satish Duggana <sa...@gmail.com>]

Hi Colin,
Thanks for the KIP. Optional fields and var length encoding support is a great
improvement for the protocol.

>>Optional fields can have any type, except that they cannot be arrays.
Note that the restriction against having tagged arrays is just to simplify
serialization.  We can relax this restriction in the future without changing
the protocol on the wire.

Can an Optional field have a struct type which internally contains an array
field at any level?

Thanks,
Satish.



On Tue, Aug 13, 2019 at 11:49 PM David Jacot <dj...@confluent.io> wrote:
>
> Hi Colin,
>
> Thank you for the KIP! Things are well explained!. It is huge improvement
> for the Kafka protocol. I have few comments on the proposal:
>
> 1. The interleaved tag/length header sounds like a great optimisation as it
> would be shorter on average. The downside, as
> you already pointed out, is that it makes the decoding and the specs more
> complex. Personally, I would also favour using two
> vaints in this particular case to keep things simple.
>
> 2. As discussed, I wonder if it would make sense to extend to KIP to also
> support optional fields in the Record Header. I think
> that it could be interesting to have such capability for common fields
> across all the requests or responses (e.g. tracing id).
>
> Regards,
> David
>
>
>
> On Tue, Aug 13, 2019 at 10:00 AM Jason Gustafson <ja...@confluent.io> wrote:
>
> > > Right, I was planning on doing exactly that for all the auto-generated
> > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > better use of time to convert the manual ones to auto gen first (with the
> > possible exception of Fetch/Produce, where the ROI may be higher for the
> > manual work)
> >
> > Yeah, that makes sense. Maybe we can include the version bump for all RPCs
> > in this KIP, but we can implement it lazily as the protocols are converted.
> >
> > -Jason
> >
> > On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > > Hi Colin,
> > > >
> > > > Thanks for the KIP! This is a significant improvement. One of my
> > personal
> > > > interests in this proposal is solving the compatibility problems we
> > have
> > > > with the internal schemas used to define consumer offsets and
> > transaction
> > > > metadata. Currently we have to guard schema bumps with the inter-broker
> > > > protocol format. Once the format is bumped, there is no way to
> > downgrade.
> > > > By fixing this, we can potentially begin using the new schemas before
> > the
> > > > IBP is bumped while still allowing downgrade.
> > > >
> > > > There are a surprising number of other situations we have encountered
> > > this
> > > > sort of problem. We have hacked around it in special cases by allowing
> > > > nullable fields to the end of the schema, but this is not really an
> > > > extensible approach. I'm looking forward to having a better option.
> > >
> > > Yeah, this problem keeps coming up.
> > >
> > > >
> > > > With that said, I have a couple questions on the proposal:
> > > >
> > > > 1. For each request API, we need one version bump to begin support for
> > > > "flexible versions." Until then, we won't have the option of using
> > tagged
> > > > fields even if the broker knows how to handle them. Does it make sense
> > to
> > > > go ahead and do a universal bump of each request API now so that we'll
> > > have
> > > > this option going forward?
> > >
> > > Right, I was planning on doing exactly that for all the auto-generated
> > > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > > better use of time to convert the manual ones to auto gen first (with the
> > > possible exception of Fetch/Produce, where the ROI may be higher for the
> > > manual work)
> > >
> > > > 2. The alternating length/tag header encoding lets us save a byte in
> > the
> > > > common case. The downside is that it's a bit more complex to specify.
> > It
> > > > also has some extra cost if the length exceeds the tag substantially.
> > > > Basically we'd have to pad the tag, right? I think I'm wondering if we
> > > > should just bite the bullet and use two varints instead.
> > >
> > > That’s a fair point. It would be shorter on average, but worse for some
> > > exceptional cases. Also, the decoding would be more complex, which might
> > be
> > > a good reason to go for just having two varints. Yeah, let’s simplify.
> > >
> > > Regards,
> > > Colin
> > >
> > > >
> > > > Thanks,
> > > > Jason
> > > >
> > > >
> > > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <cm...@apache.org>
> > wrote:
> > > >
> > > > > Hi all,
> > > > >
> > > > > I've made some updates to this KIP. Specifically, I wanted to avoid
> > > > > including escape bytes in the serialization format, since it was too
> > > > > complex. Also, I think this is a good opportunity to slim down our
> > > > > variable length fields.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > > > > > > Thanks Colin for the KIP. For my own edification why are we doing
> > > this
> > > > > > > "Optional fields can have any type, except for an array of
> > > > > structures."?
> > > > > > > Why can't we have an array of structures?
> > > > > >
> > > > > > Optional fields are serialized starting with their total length.
> > This
> > > > > > is straightforward to calculate for primitive fields like INT32,
> > (or
> > > > > > even an array of INT32), but more difficult to calculate for an
> > array
> > > > > > of structures. Basically, we'd have to do a two-pass serialization
> > > > > > where we first calculate the lengths of everything, and then write
> > it
> > > > > > out.
> > > > > >
> > > > > > The nice thing about this KIP is that there's nothing in the
> > protocol
> > > > > > stopping us from adding support for this feature in the future. We
> > > > > > wouldn't have to really change the protocol at all to add support.
> > > But
> > > > > > we'd have to change a lot of serialization code. Given almost all
> > of
> > > > > > our use-cases for optional fields are adding an extra field here or
> > > > > > there, it seems reasonable not to support it for right now.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > -Jose
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[David Jacot <dj...@confluent.io>]

Hi Colin,

Thank you for the KIP! Things are well explained!. It is huge improvement
for the Kafka protocol. I have few comments on the proposal:

1. The interleaved tag/length header sounds like a great optimisation as it
would be shorter on average. The downside, as
you already pointed out, is that it makes the decoding and the specs more
complex. Personally, I would also favour using two
vaints in this particular case to keep things simple.

2. As discussed, I wonder if it would make sense to extend to KIP to also
support optional fields in the Record Header. I think
that it could be interesting to have such capability for common fields
across all the requests or responses (e.g. tracing id).

Regards,
David



On Tue, Aug 13, 2019 at 10:00 AM Jason Gustafson <ja...@confluent.io> wrote:

> > Right, I was planning on doing exactly that for all the auto-generated
> RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> better use of time to convert the manual ones to auto gen first (with the
> possible exception of Fetch/Produce, where the ROI may be higher for the
> manual work)
>
> Yeah, that makes sense. Maybe we can include the version bump for all RPCs
> in this KIP, but we can implement it lazily as the protocols are converted.
>
> -Jason
>
> On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org> wrote:
>
> > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > Hi Colin,
> > >
> > > Thanks for the KIP! This is a significant improvement. One of my
> personal
> > > interests in this proposal is solving the compatibility problems we
> have
> > > with the internal schemas used to define consumer offsets and
> transaction
> > > metadata. Currently we have to guard schema bumps with the inter-broker
> > > protocol format. Once the format is bumped, there is no way to
> downgrade.
> > > By fixing this, we can potentially begin using the new schemas before
> the
> > > IBP is bumped while still allowing downgrade.
> > >
> > > There are a surprising number of other situations we have encountered
> > this
> > > sort of problem. We have hacked around it in special cases by allowing
> > > nullable fields to the end of the schema, but this is not really an
> > > extensible approach. I'm looking forward to having a better option.
> >
> > Yeah, this problem keeps coming up.
> >
> > >
> > > With that said, I have a couple questions on the proposal:
> > >
> > > 1. For each request API, we need one version bump to begin support for
> > > "flexible versions." Until then, we won't have the option of using
> tagged
> > > fields even if the broker knows how to handle them. Does it make sense
> to
> > > go ahead and do a universal bump of each request API now so that we'll
> > have
> > > this option going forward?
> >
> > Right, I was planning on doing exactly that for all the auto-generated
> > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > better use of time to convert the manual ones to auto gen first (with the
> > possible exception of Fetch/Produce, where the ROI may be higher for the
> > manual work)
> >
> > > 2. The alternating length/tag header encoding lets us save a byte in
> the
> > > common case. The downside is that it's a bit more complex to specify.
> It
> > > also has some extra cost if the length exceeds the tag substantially.
> > > Basically we'd have to pad the tag, right? I think I'm wondering if we
> > > should just bite the bullet and use two varints instead.
> >
> > That’s a fair point. It would be shorter on average, but worse for some
> > exceptional cases. Also, the decoding would be more complex, which might
> be
> > a good reason to go for just having two varints. Yeah, let’s simplify.
> >
> > Regards,
> > Colin
> >
> > >
> > > Thanks,
> > > Jason
> > >
> > >
> > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <cm...@apache.org>
> wrote:
> > >
> > > > Hi all,
> > > >
> > > > I've made some updates to this KIP. Specifically, I wanted to avoid
> > > > including escape bytes in the serialization format, since it was too
> > > > complex. Also, I think this is a good opportunity to slim down our
> > > > variable length fields.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > >
> > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > > > > > Thanks Colin for the KIP. For my own edification why are we doing
> > this
> > > > > > "Optional fields can have any type, except for an array of
> > > > structures."?
> > > > > > Why can't we have an array of structures?
> > > > >
> > > > > Optional fields are serialized starting with their total length.
> This
> > > > > is straightforward to calculate for primitive fields like INT32,
> (or
> > > > > even an array of INT32), but more difficult to calculate for an
> array
> > > > > of structures. Basically, we'd have to do a two-pass serialization
> > > > > where we first calculate the lengths of everything, and then write
> it
> > > > > out.
> > > > >
> > > > > The nice thing about this KIP is that there's nothing in the
> protocol
> > > > > stopping us from adding support for this feature in the future. We
> > > > > wouldn't have to really change the protocol at all to add support.
> > But
> > > > > we'd have to change a lot of serialization code. Given almost all
> of
> > > > > our use-cases for optional fields are adding an extra field here or
> > > > > there, it seems reasonable not to support it for right now.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > > >
> > > > > > --
> > > > > > -Jose
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Colin McCabe <cm...@apache.org>]

On Tue, Aug 13, 2019, at 10:01, Jason Gustafson wrote:
> > Right, I was planning on doing exactly that for all the auto-generated
> > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > better use of time to convert the manual ones to auto gen first (with the
> > possible exception of Fetch/Produce, where the ROI may be higher for the
> > manual work)
> 
> Yeah, that makes sense. Maybe we can include the version bump for all RPCs
> in this KIP, but we can implement it lazily as the protocols are converted.

Good idea.

best,
Colin

> 
> -Jason
> 
> On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org> wrote:
> 
> > On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > > Hi Colin,
> > >
> > > Thanks for the KIP! This is a significant improvement. One of my personal
> > > interests in this proposal is solving the compatibility problems we have
> > > with the internal schemas used to define consumer offsets and transaction
> > > metadata. Currently we have to guard schema bumps with the inter-broker
> > > protocol format. Once the format is bumped, there is no way to downgrade.
> > > By fixing this, we can potentially begin using the new schemas before the
> > > IBP is bumped while still allowing downgrade.
> > >
> > > There are a surprising number of other situations we have encountered
> > this
> > > sort of problem. We have hacked around it in special cases by allowing
> > > nullable fields to the end of the schema, but this is not really an
> > > extensible approach. I'm looking forward to having a better option.
> >
> > Yeah, this problem keeps coming up.
> >
> > >
> > > With that said, I have a couple questions on the proposal:
> > >
> > > 1. For each request API, we need one version bump to begin support for
> > > "flexible versions." Until then, we won't have the option of using tagged
> > > fields even if the broker knows how to handle them. Does it make sense to
> > > go ahead and do a universal bump of each request API now so that we'll
> > have
> > > this option going forward?
> >
> > Right, I was planning on doing exactly that for all the auto-generated
> > RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> > better use of time to convert the manual ones to auto gen first (with the
> > possible exception of Fetch/Produce, where the ROI may be higher for the
> > manual work)
> >
> > > 2. The alternating length/tag header encoding lets us save a byte in the
> > > common case. The downside is that it's a bit more complex to specify. It
> > > also has some extra cost if the length exceeds the tag substantially.
> > > Basically we'd have to pad the tag, right? I think I'm wondering if we
> > > should just bite the bullet and use two varints instead.
> >
> > That’s a fair point. It would be shorter on average, but worse for some
> > exceptional cases. Also, the decoding would be more complex, which might be
> > a good reason to go for just having two varints. Yeah, let’s simplify.
> >
> > Regards,
> > Colin
> >
> > >
> > > Thanks,
> > > Jason
> > >
> > >
> > > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <cm...@apache.org> wrote:
> > >
> > > > Hi all,
> > > >
> > > > I've made some updates to this KIP. Specifically, I wanted to avoid
> > > > including escape bytes in the serialization format, since it was too
> > > > complex. Also, I think this is a good opportunity to slim down our
> > > > variable length fields.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > >
> > > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > > > > > Thanks Colin for the KIP. For my own edification why are we doing
> > this
> > > > > > "Optional fields can have any type, except for an array of
> > > > structures."?
> > > > > > Why can't we have an array of structures?
> > > > >
> > > > > Optional fields are serialized starting with their total length. This
> > > > > is straightforward to calculate for primitive fields like INT32, (or
> > > > > even an array of INT32), but more difficult to calculate for an array
> > > > > of structures. Basically, we'd have to do a two-pass serialization
> > > > > where we first calculate the lengths of everything, and then write it
> > > > > out.
> > > > >
> > > > > The nice thing about this KIP is that there's nothing in the protocol
> > > > > stopping us from adding support for this feature in the future. We
> > > > > wouldn't have to really change the protocol at all to add support.
> > But
> > > > > we'd have to change a lot of serialization code. Given almost all of
> > > > > our use-cases for optional fields are adding an extra field here or
> > > > > there, it seems reasonable not to support it for right now.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > > >
> > > > > > --
> > > > > > -Jose
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Jason Gustafson <ja...@confluent.io>]

> Right, I was planning on doing exactly that for all the auto-generated
RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
better use of time to convert the manual ones to auto gen first (with the
possible exception of Fetch/Produce, where the ROI may be higher for the
manual work)

Yeah, that makes sense. Maybe we can include the version bump for all RPCs
in this KIP, but we can implement it lazily as the protocols are converted.

-Jason

On Mon, Aug 12, 2019 at 7:16 PM Colin McCabe <cm...@apache.org> wrote:

> On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> > Hi Colin,
> >
> > Thanks for the KIP! This is a significant improvement. One of my personal
> > interests in this proposal is solving the compatibility problems we have
> > with the internal schemas used to define consumer offsets and transaction
> > metadata. Currently we have to guard schema bumps with the inter-broker
> > protocol format. Once the format is bumped, there is no way to downgrade.
> > By fixing this, we can potentially begin using the new schemas before the
> > IBP is bumped while still allowing downgrade.
> >
> > There are a surprising number of other situations we have encountered
> this
> > sort of problem. We have hacked around it in special cases by allowing
> > nullable fields to the end of the schema, but this is not really an
> > extensible approach. I'm looking forward to having a better option.
>
> Yeah, this problem keeps coming up.
>
> >
> > With that said, I have a couple questions on the proposal:
> >
> > 1. For each request API, we need one version bump to begin support for
> > "flexible versions." Until then, we won't have the option of using tagged
> > fields even if the broker knows how to handle them. Does it make sense to
> > go ahead and do a universal bump of each request API now so that we'll
> have
> > this option going forward?
>
> Right, I was planning on doing exactly that for all the auto-generated
> RPCs. For the manual RPCs, it would be a lot of work. It’s probably a
> better use of time to convert the manual ones to auto gen first (with the
> possible exception of Fetch/Produce, where the ROI may be higher for the
> manual work)
>
> > 2. The alternating length/tag header encoding lets us save a byte in the
> > common case. The downside is that it's a bit more complex to specify. It
> > also has some extra cost if the length exceeds the tag substantially.
> > Basically we'd have to pad the tag, right? I think I'm wondering if we
> > should just bite the bullet and use two varints instead.
>
> That’s a fair point. It would be shorter on average, but worse for some
> exceptional cases. Also, the decoding would be more complex, which might be
> a good reason to go for just having two varints. Yeah, let’s simplify.
>
> Regards,
> Colin
>
> >
> > Thanks,
> > Jason
> >
> >
> > On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > > Hi all,
> > >
> > > I've made some updates to this KIP. Specifically, I wanted to avoid
> > > including escape bytes in the serialization format, since it was too
> > > complex. Also, I think this is a good opportunity to slim down our
> > > variable length fields.
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > > > > Thanks Colin for the KIP. For my own edification why are we doing
> this
> > > > > "Optional fields can have any type, except for an array of
> > > structures."?
> > > > > Why can't we have an array of structures?
> > > >
> > > > Optional fields are serialized starting with their total length. This
> > > > is straightforward to calculate for primitive fields like INT32, (or
> > > > even an array of INT32), but more difficult to calculate for an array
> > > > of structures. Basically, we'd have to do a two-pass serialization
> > > > where we first calculate the lengths of everything, and then write it
> > > > out.
> > > >
> > > > The nice thing about this KIP is that there's nothing in the protocol
> > > > stopping us from adding support for this feature in the future. We
> > > > wouldn't have to really change the protocol at all to add support.
> But
> > > > we'd have to change a lot of serialization code. Given almost all of
> > > > our use-cases for optional fields are adding an extra field here or
> > > > there, it seems reasonable not to support it for right now.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > > >
> > > > > --
> > > > > -Jose
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Colin McCabe <cm...@apache.org>]

On Mon, Aug 12, 2019, at 11:22, Jason Gustafson wrote:
> Hi Colin,
> 
> Thanks for the KIP! This is a significant improvement. One of my personal
> interests in this proposal is solving the compatibility problems we have
> with the internal schemas used to define consumer offsets and transaction
> metadata. Currently we have to guard schema bumps with the inter-broker
> protocol format. Once the format is bumped, there is no way to downgrade.
> By fixing this, we can potentially begin using the new schemas before the
> IBP is bumped while still allowing downgrade.
> 
> There are a surprising number of other situations we have encountered this
> sort of problem. We have hacked around it in special cases by allowing
> nullable fields to the end of the schema, but this is not really an
> extensible approach. I'm looking forward to having a better option.

Yeah, this problem keeps coming up.

> 
> With that said, I have a couple questions on the proposal:
> 
> 1. For each request API, we need one version bump to begin support for
> "flexible versions." Until then, we won't have the option of using tagged
> fields even if the broker knows how to handle them. Does it make sense to
> go ahead and do a universal bump of each request API now so that we'll have
> this option going forward?

Right, I was planning on doing exactly that for all the auto-generated RPCs. For the manual RPCs, it would be a lot of work. It’s probably a better use of time to convert the manual ones to auto gen first (with the possible exception of Fetch/Produce, where the ROI may be higher for the manual work)

> 2. The alternating length/tag header encoding lets us save a byte in the
> common case. The downside is that it's a bit more complex to specify. It
> also has some extra cost if the length exceeds the tag substantially.
> Basically we'd have to pad the tag, right? I think I'm wondering if we
> should just bite the bullet and use two varints instead.

That’s a fair point. It would be shorter on average, but worse for some exceptional cases. Also, the decoding would be more complex, which might be a good reason to go for just having two varints. Yeah, let’s simplify.

Regards,
Colin

> 
> Thanks,
> Jason
> 
> 
> On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <cm...@apache.org> wrote:
> 
> > Hi all,
> >
> > I've made some updates to this KIP. Specifically, I wanted to avoid
> > including escape bytes in the serialization format, since it was too
> > complex. Also, I think this is a good opportunity to slim down our
> > variable length fields.
> >
> > best,
> > Colin
> >
> >
> > On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > > > Thanks Colin for the KIP. For my own edification why are we doing this
> > > > "Optional fields can have any type, except for an array of
> > structures."?
> > > > Why can't we have an array of structures?
> > >
> > > Optional fields are serialized starting with their total length. This
> > > is straightforward to calculate for primitive fields like INT32, (or
> > > even an array of INT32), but more difficult to calculate for an array
> > > of structures. Basically, we'd have to do a two-pass serialization
> > > where we first calculate the lengths of everything, and then write it
> > > out.
> > >
> > > The nice thing about this KIP is that there's nothing in the protocol
> > > stopping us from adding support for this feature in the future. We
> > > wouldn't have to really change the protocol at all to add support. But
> > > we'd have to change a lot of serialization code. Given almost all of
> > > our use-cases for optional fields are adding an extra field here or
> > > there, it seems reasonable not to support it for right now.
> > >
> > > best,
> > > Colin
> > >
> > > >
> > > > --
> > > > -Jose
> > > >
> > >
> >
> 

Re: [DISCUSS] KIP-482: The Kafka Protocol should Support Optional Fields

[Jason Gustafson <ja...@confluent.io>]

Hi Colin,

Thanks for the KIP! This is a significant improvement. One of my personal
interests in this proposal is solving the compatibility problems we have
with the internal schemas used to define consumer offsets and transaction
metadata. Currently we have to guard schema bumps with the inter-broker
protocol format. Once the format is bumped, there is no way to downgrade.
By fixing this, we can potentially begin using the new schemas before the
IBP is bumped while still allowing downgrade.

There are a surprising number of other situations we have encountered this
sort of problem. We have hacked around it in special cases by allowing
nullable fields to the end of the schema, but this is not really an
extensible approach. I'm looking forward to having a better option.

With that said, I have a couple questions on the proposal:

1. For each request API, we need one version bump to begin support for
"flexible versions." Until then, we won't have the option of using tagged
fields even if the broker knows how to handle them. Does it make sense to
go ahead and do a universal bump of each request API now so that we'll have
this option going forward?
2. The alternating length/tag header encoding lets us save a byte in the
common case. The downside is that it's a bit more complex to specify. It
also has some extra cost if the length exceeds the tag substantially.
Basically we'd have to pad the tag, right? I think I'm wondering if we
should just bite the bullet and use two varints instead.

Thanks,
Jason


On Fri, Aug 9, 2019 at 4:31 PM Colin McCabe <cm...@apache.org> wrote:

> Hi all,
>
> I've made some updates to this KIP.  Specifically, I wanted to avoid
> including escape bytes in the serialization format, since it was too
> complex.  Also, I think this is a good opportunity to slim down our
> variable length fields.
>
> best,
> Colin
>
>
> On Thu, Jul 11, 2019, at 20:52, Colin McCabe wrote:
> > On Tue, Jul 9, 2019, at 15:29, Jose Armando Garcia Sancio wrote:
> > > Thanks Colin for the KIP. For my own edification why are we doing this
> > > "Optional fields can have any type, except for an array of
> structures."?
> > > Why can't we have an array of structures?
> >
> > Optional fields are serialized starting with their total length.  This
> > is straightforward to calculate for primitive fields like INT32, (or
> > even an array of INT32), but more difficult to calculate for an array
> > of structures.  Basically, we'd have to do a two-pass serialization
> > where we first calculate the lengths of everything, and then write it
> > out.
> >
> > The nice thing about this KIP is that there's nothing in the protocol
> > stopping us from adding support for this feature in the future.  We
> > wouldn't have to really change the protocol at all to add support.  But
> > we'd have to change a lot of serialization code.  Given almost all of
> > our use-cases for optional fields are adding an extra field here or
> > there, it seems reasonable not to support it for right now.
> >
> > best,
> > Colin
> >
> > >
> > > --
> > > -Jose
> > >
> >
>