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).
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? 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". 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. 3. Did we consider using `session` (like the group coordinator) instead of `regsitration` in `broker.registration.timeout.ms`? 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. 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. 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. 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? 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`. 10. Should `UnregisterBrokerRecord` be `DeregisterBrokerRecord`? 11. Broker metrics typically have a PerSec suffix, should we stick with that for the `MetadataCommitRate`? 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. 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 >
