You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Colin McCabe <cm...@apache.org> on 2021/01/06 01:03:10 UTC
Re: [VOTE] voting on KIP-631: the quorum-based Kafka controller
Hi all,
Addendum: some of the port types in this KIP were specified as int16 in the wire protocol. But this does not gracefully handle ports like 33,000, which shows up as negative when using a signed 16 bit number. I think eventually we'll want a uint16 type, but for now I just made them int32. This is consistent with what we do in MetadataResponse and a few other places.
best,
Colin
On Mon, Dec 21, 2020, at 14:42, Colin McCabe wrote:
> Hi all,
>
> With non-binding +1 votes from Ron Dagostino, Tom Bentley and Unmesh
> Joshi, and binding +1 votes from David Arthur, Boyang Chen, Jason
> Gusafson, Ismael Juma, David Jacot, Jun Rao, the KIP passes.
>
> thanks, all!
>
> cheers,
> Colin
>
> On Fri, Dec 18, 2020, at 12:42, Colin McCabe wrote:
> > Hi all,
> >
> > I'm going to close the vote in a few hours. Thanks to everyone who
> > reviewed and voted.
> >
> > best,
> > Colin
> >
> >
> > On Fri, Dec 18, 2020, at 10:08, Jun Rao wrote:
> > > Thanks, Colin. +1
> > >
> > > Jun
> > >
> > > On Thu, Dec 17, 2020 at 2:24 AM David Jacot <dj...@confluent.io> wrote:
> > >
> > > > Thanks for driving this KIP, Colin. The KIP is really well written. This is
> > > > so exciting!
> > > >
> > > > +1 (binding)
> > > >
> > > > Best,
> > > > David
> > > >
> > > > On Wed, Dec 16, 2020 at 11:51 PM Colin McCabe <cm...@apache.org> wrote:
> > > >
> > > > > On Wed, Dec 16, 2020, at 13:08, Ismael Juma wrote:
> > > > > > Thanks for all the work on the KIP. Given the magnitude of the KIP, I
> > > > > > expect that some tweaks will be made as the code is implemented,
> > > > reviewed
> > > > > > and tested. I'm overall +1 (binding).
> > > > > >
> > > > >
> > > > > Thanks, Ismael.
> > > > >
> > > > > > A few comments below:
> > > > > > 1. It's a bit weird for kafka-storage to output a random uuid. Would it
> > > > > be
> > > > > > better to have a dedicated command for that?
> > > > >
> > > > > I'm not sure. The nice thing about putting it in kafka-storage.sh is
> > > > that
> > > > > it's there when you need it. I also think that having subcommands, like
> > > > we
> > > > > do here, really reduces the "clutter" that we have in some other
> > > > > command-line tools. When you get help about the "info" subcommand, you
> > > > > don't see flags for any other subcommand, for example. I guess we can
> > > > move
> > > > > this later if it seems more intuitive though.
> > > > >
> > > > > > Also, since we use base64
> > > > > > encoded uuids nearly everywhere (including cluster and topic ids), it
> > > > > would
> > > > > > be good to follow that pattern instead of the less compact
> > > > > > "51380268-1036-410d-a8fc-fb3b55f48033".
> > > > >
> > > > > Good idea. I have updated this to use base64 encoded UUIDs.
> > > > >
> > > > > > 2. This is a nit, but I think it would be better to talk about built-in
> > > > > > quorum mode instead of KIP-500 mode. It's more self descriptive than a
> > > > > KIP
> > > > > > reference.
> > > > >
> > > > > I do like the sound of "quorum mode." I guess the main question is, if
> > > > we
> > > > > later implement raft quorums for regular topics, would that nomenclature
> > > > be
> > > > > confusing? I guess we could talk about "metadata quorum mode" to avoid
> > > > > confusion. Hmm.
> > > > >
> > > > > > 3. Did we consider using `session` (like the group coordinator) instead
> > > > > of
> > > > > > `regsitration` in `broker.registration.timeout.ms`?
> > > > >
> > > > > Hmm, broker.session.timeout.ms does sound better. I changed it to that.
> > > > >
> > > > > > 4. The flat id space for the controller and broker while requiring a
> > > > > > different id in embedded mode seems a bit unintuitive. Are there any
> > > > > other
> > > > > > systems that do this? I know we covered some of the reasons in the
> > > > > "Shared
> > > > > > IDs between Multiple Nodes" rejected alternatives section, but it
> > > > didn't
> > > > > > seem totally convincing to me.
> > > > >
> > > > > One of my concerns here is that using separate ID spaces for controllers
> > > > > versus brokers would potentially lead to metrics or logging collisions.
> > > > We
> > > > > can take a look at that again once the implementation is further along, I
> > > > > guess, to see how often that is a problem in practice.
> > > > >
> > > > > > 5. With regards to the controller process listening on a separate port,
> > > > > it
> > > > > > may be worth adding a sentence about the forwarding KIP as that is a
> > > > main
> > > > > > reason why the controller port doesn't need to be accessible.
> > > > >
> > > > > Good idea... I added a short reference to KIP-590 in the "Networking"
> > > > > section.
> > > > >
> > > > > > 6. The internal topic seems to be called @metadata. I'm personally not
> > > > > > convinced about the usage of @ in this way. I think I would go with the
> > > > > > same convention we have used for other existing internal topics.
> > > > >
> > > > > I knew this one would be controversial :)
> > > > >
> > > > > I guess the main argument here is that using @ avoids collisions with any
> > > > > existing topic. Leading underscores, even double underscores, can be
> > > > used
> > > > > by users to create new topics, but an "at sign" cannot It would be nice
> > > > to
> > > > > have a namespace for system topics that we knew nobody else could break
> > > > > into.
> > > > >
> > > > > > 7. We talk about the metadata.format feature flag. Is this intended to
> > > > > > allow for single roll upgrades?
> > > > > > 8. Could the incarnation id be called registration id? Or is there a
> > > > > reason
> > > > > > why this would be a bad name?
> > > > >
> > > > > I liked "incarnation id" because it expresses the idea that each new
> > > > > incarnation of the broker gets a different one. I think "registration
> > > > id"
> > > > > might be confused with "the broker id is the ID we're registering."
> > > > >
> > > > > > 9. Could `CurMetadataOffset` be called `CurrentMetadataOffset` for
> > > > > > `BrokerRegistrationRequest`? The abbreviation here doesn't seem to help
> > > > > > much and makes things slightly less readable. It would also make it
> > > > > > consistent with `BrokerHeartbeatRequest`.
> > > > >
> > > > > Yeah, the abbreviated name is inconsistent. I will change it to
> > > > > CurrentMetadataOffset.
> > > > >
> > > > > > 10. Should `UnregisterBrokerRecord` be `DeregisterBrokerRecord`?
> > > > >
> > > > > Hmm, "Register/Unregister" is more consistent with "Fence/Unfence" which
> > > > > is why I went with Unregister. It looks like they're both in the
> > > > > dictionary, so I'm not sure if "deregister" has an advantage...
> > > > >
> > > > > > 11. Broker metrics typically have a PerSec suffix, should we stick with
> > > > > > that for the `MetadataCommitRate`?
> > > > >
> > > > > Added.
> > > > >
> > > > > > 12. For the lag metrics, would it be clearer if we included "Offset" in
> > > > > the
> > > > > > name? In theory, we could have time based lag metrics too. Having said
> > > > > > that, existing offset lag metrics do seem to just have `Lag` in their
> > > > > name
> > > > > > without further qualification.
> > > > > >
> > > > >
> > > > > Yeah, I think including Offset does make it a bit clearer. Added.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > > Ismael
> > > > > >
> > > > > > On Fri, Dec 11, 2020 at 1:41 PM Colin McCabe <cm...@apache.org>
> > > > wrote:
> > > > > >
> > > > > > > Hi all,
> > > > > > >
> > > > > > > I'd like to restart the vote on KIP-631: the quorum-based Kafka
> > > > > > > Controller. The KIP is here:
> > > > > > >
> > > > > > > https://cwiki.apache.org/confluence/x/4RV4CQ
> > > > > > >
> > > > > > > The original DISCUSS thread is here:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > > > https://lists.apache.org/thread.html/r1ed098a88c489780016d963b065e8cb450a9080a4736457cd25f323c%40%3Cdev.kafka.apache.org%3E
> > > > > > >
> > > > > > > There is also a second email DISCUSS thread, which is here:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > > > https://lists.apache.org/thread.html/r1ed098a88c489780016d963b065e8cb450a9080a4736457cd25f323c%40%3Cdev.kafka.apache.org%3E
> > > > > > >
> > > > > > > Please take a look and vote if you can.
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [VOTE] voting on KIP-631: the quorum-based Kafka controller
Posted by Colin McCabe <cm...@apache.org>.
After thinking about this more, I have decided to just use an unsigned 16-bit int for ports. Using 4 bytes would be wasteful here. So I set these fields to uint16. This is straightforward to support in the wire protocol and will be more efficient going forward. I updated the KIP.
best,
Colin
On Tue, Jan 5, 2021, at 17:03, Colin McCabe wrote:
> Hi all,
>
> Addendum: some of the port types in this KIP were specified as int16 in
> the wire protocol. But this does not gracefully handle ports like
> 33,000, which shows up as negative when using a signed 16 bit number.
> I think eventually we'll want a uint16 type, but for now I just made
> them int32. This is consistent with what we do in MetadataResponse and
> a few other places.
>
> best,
> Colin
>
>
> On Mon, Dec 21, 2020, at 14:42, Colin McCabe wrote:
> > Hi all,
> >
> > With non-binding +1 votes from Ron Dagostino, Tom Bentley and Unmesh
> > Joshi, and binding +1 votes from David Arthur, Boyang Chen, Jason
> > Gusafson, Ismael Juma, David Jacot, Jun Rao, the KIP passes.
> >
> > thanks, all!
> >
> > cheers,
> > Colin
> >
> > On Fri, Dec 18, 2020, at 12:42, Colin McCabe wrote:
> > > Hi all,
> > >
> > > I'm going to close the vote in a few hours. Thanks to everyone who
> > > reviewed and voted.
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Fri, Dec 18, 2020, at 10:08, Jun Rao wrote:
> > > > Thanks, Colin. +1
> > > >
> > > > Jun
> > > >
> > > > On Thu, Dec 17, 2020 at 2:24 AM David Jacot <dj...@confluent.io> wrote:
> > > >
> > > > > Thanks for driving this KIP, Colin. The KIP is really well written. This is
> > > > > so exciting!
> > > > >
> > > > > +1 (binding)
> > > > >
> > > > > Best,
> > > > > David
> > > > >
> > > > > On Wed, Dec 16, 2020 at 11:51 PM Colin McCabe <cm...@apache.org> wrote:
> > > > >
> > > > > > On Wed, Dec 16, 2020, at 13:08, Ismael Juma wrote:
> > > > > > > Thanks for all the work on the KIP. Given the magnitude of the KIP, I
> > > > > > > expect that some tweaks will be made as the code is implemented,
> > > > > reviewed
> > > > > > > and tested. I'm overall +1 (binding).
> > > > > > >
> > > > > >
> > > > > > Thanks, Ismael.
> > > > > >
> > > > > > > A few comments below:
> > > > > > > 1. It's a bit weird for kafka-storage to output a random uuid. Would it
> > > > > > be
> > > > > > > better to have a dedicated command for that?
> > > > > >
> > > > > > I'm not sure. The nice thing about putting it in kafka-storage.sh is
> > > > > that
> > > > > > it's there when you need it. I also think that having subcommands, like
> > > > > we
> > > > > > do here, really reduces the "clutter" that we have in some other
> > > > > > command-line tools. When you get help about the "info" subcommand, you
> > > > > > don't see flags for any other subcommand, for example. I guess we can
> > > > > move
> > > > > > this later if it seems more intuitive though.
> > > > > >
> > > > > > > Also, since we use base64
> > > > > > > encoded uuids nearly everywhere (including cluster and topic ids), it
> > > > > > would
> > > > > > > be good to follow that pattern instead of the less compact
> > > > > > > "51380268-1036-410d-a8fc-fb3b55f48033".
> > > > > >
> > > > > > Good idea. I have updated this to use base64 encoded UUIDs.
> > > > > >
> > > > > > > 2. This is a nit, but I think it would be better to talk about built-in
> > > > > > > quorum mode instead of KIP-500 mode. It's more self descriptive than a
> > > > > > KIP
> > > > > > > reference.
> > > > > >
> > > > > > I do like the sound of "quorum mode." I guess the main question is, if
> > > > > we
> > > > > > later implement raft quorums for regular topics, would that nomenclature
> > > > > be
> > > > > > confusing? I guess we could talk about "metadata quorum mode" to avoid
> > > > > > confusion. Hmm.
> > > > > >
> > > > > > > 3. Did we consider using `session` (like the group coordinator) instead
> > > > > > of
> > > > > > > `regsitration` in `broker.registration.timeout.ms`?
> > > > > >
> > > > > > Hmm, broker.session.timeout.ms does sound better. I changed it to that.
> > > > > >
> > > > > > > 4. The flat id space for the controller and broker while requiring a
> > > > > > > different id in embedded mode seems a bit unintuitive. Are there any
> > > > > > other
> > > > > > > systems that do this? I know we covered some of the reasons in the
> > > > > > "Shared
> > > > > > > IDs between Multiple Nodes" rejected alternatives section, but it
> > > > > didn't
> > > > > > > seem totally convincing to me.
> > > > > >
> > > > > > One of my concerns here is that using separate ID spaces for controllers
> > > > > > versus brokers would potentially lead to metrics or logging collisions.
> > > > > We
> > > > > > can take a look at that again once the implementation is further along, I
> > > > > > guess, to see how often that is a problem in practice.
> > > > > >
> > > > > > > 5. With regards to the controller process listening on a separate port,
> > > > > > it
> > > > > > > may be worth adding a sentence about the forwarding KIP as that is a
> > > > > main
> > > > > > > reason why the controller port doesn't need to be accessible.
> > > > > >
> > > > > > Good idea... I added a short reference to KIP-590 in the "Networking"
> > > > > > section.
> > > > > >
> > > > > > > 6. The internal topic seems to be called @metadata. I'm personally not
> > > > > > > convinced about the usage of @ in this way. I think I would go with the
> > > > > > > same convention we have used for other existing internal topics.
> > > > > >
> > > > > > I knew this one would be controversial :)
> > > > > >
> > > > > > I guess the main argument here is that using @ avoids collisions with any
> > > > > > existing topic. Leading underscores, even double underscores, can be
> > > > > used
> > > > > > by users to create new topics, but an "at sign" cannot It would be nice
> > > > > to
> > > > > > have a namespace for system topics that we knew nobody else could break
> > > > > > into.
> > > > > >
> > > > > > > 7. We talk about the metadata.format feature flag. Is this intended to
> > > > > > > allow for single roll upgrades?
> > > > > > > 8. Could the incarnation id be called registration id? Or is there a
> > > > > > reason
> > > > > > > why this would be a bad name?
> > > > > >
> > > > > > I liked "incarnation id" because it expresses the idea that each new
> > > > > > incarnation of the broker gets a different one. I think "registration
> > > > > id"
> > > > > > might be confused with "the broker id is the ID we're registering."
> > > > > >
> > > > > > > 9. Could `CurMetadataOffset` be called `CurrentMetadataOffset` for
> > > > > > > `BrokerRegistrationRequest`? The abbreviation here doesn't seem to help
> > > > > > > much and makes things slightly less readable. It would also make it
> > > > > > > consistent with `BrokerHeartbeatRequest`.
> > > > > >
> > > > > > Yeah, the abbreviated name is inconsistent. I will change it to
> > > > > > CurrentMetadataOffset.
> > > > > >
> > > > > > > 10. Should `UnregisterBrokerRecord` be `DeregisterBrokerRecord`?
> > > > > >
> > > > > > Hmm, "Register/Unregister" is more consistent with "Fence/Unfence" which
> > > > > > is why I went with Unregister. It looks like they're both in the
> > > > > > dictionary, so I'm not sure if "deregister" has an advantage...
> > > > > >
> > > > > > > 11. Broker metrics typically have a PerSec suffix, should we stick with
> > > > > > > that for the `MetadataCommitRate`?
> > > > > >
> > > > > > Added.
> > > > > >
> > > > > > > 12. For the lag metrics, would it be clearer if we included "Offset" in
> > > > > > the
> > > > > > > name? In theory, we could have time based lag metrics too. Having said
> > > > > > > that, existing offset lag metrics do seem to just have `Lag` in their
> > > > > > name
> > > > > > > without further qualification.
> > > > > > >
> > > > > >
> > > > > > Yeah, I think including Offset does make it a bit clearer. Added.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > >
> > > > > > > Ismael
> > > > > > >
> > > > > > > On Fri, Dec 11, 2020 at 1:41 PM Colin McCabe <cm...@apache.org>
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > I'd like to restart the vote on KIP-631: the quorum-based Kafka
> > > > > > > > Controller. The KIP is here:
> > > > > > > >
> > > > > > > > https://cwiki.apache.org/confluence/x/4RV4CQ
> > > > > > > >
> > > > > > > > The original DISCUSS thread is here:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > > https://lists.apache.org/thread.html/r1ed098a88c489780016d963b065e8cb450a9080a4736457cd25f323c%40%3Cdev.kafka.apache.org%3E
> > > > > > > >
> > > > > > > > There is also a second email DISCUSS thread, which is here:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > > https://lists.apache.org/thread.html/r1ed098a88c489780016d963b065e8cb450a9080a4736457cd25f323c%40%3Cdev.kafka.apache.org%3E
> > > > > > > >
> > > > > > > > Please take a look and vote if you can.
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>