José, thanks for the KIP! I think this is a good approach for proving the liveness of the quorum when metadata is not changing.
1. Based on the config name "metadata.monitor.write.interval.ms" I'm guessing the intention is to have a regularly scheduled write. If the quorum is busy with lots of the writes, we wouldn't need this NoopRecord right? Maybe we can instead write a NoopRecord only after some amount of idle time. 2. A small naming suggestion, what about "NoOpRecord"? 3. Typo in one of the metric names: "MetadataWriteOffses" 4. We should consider omitting these records from the log dump tool, or at least adding an option to skip over them. 5. If (somehow) one of these NoopRecord appeared in the snapshot, it sounds like the broker/controller would skip over them. We should specify this behavior in the KIP Cheers, David On Wed, May 11, 2022 at 2:37 AM Luke Chen <show...@gmail.com> wrote: > 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é > > > -- David Arthur
