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 > > > > > > > > > >
