Hi José, Thanks for the KIP!
Some questions: 1. Jason has asked but you didn't answer: What is the default value for ` metadata.monitor.write.interval.ms`? 2. The `noopRecord` API key is `TBD`. Why can't we put the "currently used API Key nums + 1" into it? Any concern? 3. typo: name=MetadataWriteOffse[t]s 4. I don't understand the difference between MetadataLastCommittedOffset and MetadataWriteOffsets metrics. I think the 2 values will always be identical, unless the controller metadata write failed, is that correct? Thank you. Luke On Wed, May 11, 2022 at 5:58 AM José Armando García Sancio <jsan...@confluent.io.invalid> wrote: > Thanks for your feedback Jason, much appreciated. > > Here are the changes to the KIP: > > https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=211883219&selectedPageVersions=5&selectedPageVersions=4 > > On Tue, May 10, 2022 at 1:34 PM Jason Gustafson > <ja...@confluent.io.invalid> wrote: > > The approach sounds reasonable. By the way, I think one of the gaps we > have > > today is when the leader gets partitioned from the remaining voters. I > > believe it continues acting as a leader indefinitely. I was considering > > whether this periodic write can address the issue. Basically it can be > used > > to force a leader to prove it is still the leader by committing some > data. > > Say, for example, that the leader fails to commit the record after the > > fetch timeout expires, then perhaps it could start a new election. What > do > > you think? > > We have an issue for this at > https://issues.apache.org/jira/browse/KAFKA-13621. I updated the issue > with your feedback and included some of my thoughts. Do you mind if we > move this conversation to that issue? > > > A couple additional questions: > > > > - What is the default value for `metadata.monitor.write.interval.ms`? > Also, > > I'm wondering if `controller` would be a more suitable prefix? > > Yeah. I am not sure. Looking at the current configuration we have both > prefixes. For example, with the `controller` prefix we have > `controller.quorum.voters`, `controller.listener.names`, > `controller.quota.window.num`, etc. For the `metadata` prefix we have > `metadata.log.dir`, `metadata.log.*` and `metadat.max.retention.ms`, > etc. > I get the impression that we use `metadata` for things that are kinda > log/disk related and `controller` for things that are not. I am > thinking that the `metadata` prefix is more consistent with the > current situation. What do you think Jason? > > > - Could we avoid letting BrokerMetadataPublisher escape into the metric > > name? Letting the classnames leak into the metrics tends to cause > > compatibility issues over time. > > Good point. For Raft we use `kafka.server:type=raft-metrics,name=...`. > I'll change it to > `kafka.server:type=broker-metadata-metrics,name=...`. > > Thanks, > -José >
