Hi, Jose, Thanks for the reply.
54. Yes, we could include SecurityProtocol in DescribeQuorumResponse. Then, we could include it in the output of kafka-metadata-quorum --describe. 55.1 Could number-of-observers and pending-voter-change be reported by all brokers and controllers? I thought only the controller leader tracks those. 55.2 So, IgnoredStaticVoters and IsObserver are Yammer metrics and the rest are KafkaMetric. It would be useful to document the metric names clearer. For Yammer metrics, we need to specify group, type, name and tags. For KafkaMetrics, we need to specify just name and tags. 57. Could we remove --release-version 3.8 in the upgrade example? Jun On Mon, Mar 25, 2024 at 11:54 AM José Armando García Sancio <jsan...@confluent.io.invalid> wrote: > Hi Jun, > > See my comments below. > > On Fri, Mar 22, 2024 at 1:30 PM Jun Rao <j...@confluent.io.invalid> wrote: > > 54. Admin.addMetadataVoter: It seems that Endpoint shouldn't include > > securityProtocol since it's not in DescribeQuorumResponse. > > Yeah. I noticed that when I made the Admin changes. We either use a > different type in the Admin client or add SecurityProtocol to the > DescribeQuorumResponse. I was originally leaning towards adding > SecurityProtocol to the DescribeQuorumResponse. > > Does the user want to see the security protocol in the response and > CLI output? Listener name is not very useful unless the user also has > access to the controller's configuration. > > I can go either way, what do you think? > > > 55. Metrics: > > 55.1 It would be useful to be clear whether they are reported by the > > controller leader, all controllers or all controllers and brokers. > > Done. I also noticed that I was missing one metric in the controller > process role. > > > 55.2 IsObserver, type=KafkaController: Should we use the dash convention > to > > be consistent with the rest of the metrics? > > I would like to but I had to do this for backward compatibility. The > existing controller metrics are all scoped under the KafkaController > type. We had a similar discussion for "KIP-835: Monitor KRaft > Controller Quorum Health." > > > 56. kafka-storage : "If the --release-version flag is not specified, the > > IBP in the configuration is used." > > kafka-storage takes controller.properties as the input parameter and > IBP > > is not a controller property, right? > > I was documenting the current code. I suspect that the developer that > implemented kafka-storage wanted it to work with a configuration that > had an IBP. > > > 57. To be consistent with kafka-storage, should we make the > > --release-version flag in kafka-features optional too? If this is not > > specified, the default associated with the tool will be used. > > Sounds good. I updated that section to define this behavior for both > the upgrade and downgrade commands. > > > 58. Typo: when the voter ID and UUID doesn't match > > doesn't => don't > > Fixed. > > Thanks, I already updated the KIP to match my comments above and > include your feedback. > -- > -José >
