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 <dja...@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 <cmcc...@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 <cmcc...@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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
