You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/07/27 15:09:49 UTC
[GitHub] [kafka] rondagostino opened a new pull request #11133: KAFKA-13140: KRaft brokers do not expose kafka.controller metrics
rondagostino opened a new pull request #11133:
URL: https://github.com/apache/kafka/pull/11133
Several controller metrics are exposed on every broker in a ZooKeeper-based (i.e. non-KRaft) cluster regardless of whether the broker is the active controller or not, but these metrics are not exposed on KRaft nodes that have process.roles=broker (i.e. KRaft nodes that do not implement the controller role). For backwards compatibility, KRaft nodes that are just brokers should expose these metrics with values all equal to 0: just like ZooKeeper-based brokers do when they are not the active controller. This patch adds these metrics and an associated test case.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #11133: KAFKA-13140: KRaft brokers do not expose kafka.controller metrics
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #11133:
URL: https://github.com/apache/kafka/pull/11133#issuecomment-904022863
> I am not sure we should expose controller metrics for brokers that don't have the controller role.
I agree it is a bit counter-intuitive. Now that you bring up this hesitance, here is how I see it.
It *may* make sense from a backwards-compatibility perspective: those metrics are there on non-controllers today with values equal to 0, so it could be argued that for compatibility purposes they should be there for non-controller KRaft nodes.
That being said, though, the reason the controller metrics are available on ZK-based brokers today is because those brokers may soon be elected as the controller -- they are "controller-eligible". A KRaft node that has `process.roles=broker` is not controller-eligible, so that's an argument against exposing the controller metrics on KRaft broker nodes. Similarly, today we expose broker metrics on a node that is the (ZK-based) controller because that node **is** also a broker. A KRaft node with `process.roles=controller` does not currently expose broker metrics -- we have not made any such backwards-compatibility argument for this case and probably would not do so given the volume of metrics and the fact that the node it is not broker-eligible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino closed pull request #11133: KAFKA-13140: KRaft brokers do not expose kafka.controller metrics
Posted by GitBox <gi...@apache.org>.
rondagostino closed pull request #11133:
URL: https://github.com/apache/kafka/pull/11133
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dielhennr commented on a change in pull request #11133: KAFKA-13140: KRaft brokers do not expose kafka.controller metrics
Posted by GitBox <gi...@apache.org>.
dielhennr commented on a change in pull request #11133:
URL: https://github.com/apache/kafka/pull/11133#discussion_r678717206
##########
File path: core/src/main/scala/kafka/server/KafkaRaftServer.scala
##########
@@ -100,6 +101,9 @@ class KafkaRaftServer(
controllerQuorumVotersFuture
))
} else {
+ // we need to register the various kafka.controller metrics
+ // for backwards compatibility with the ZooKeeper-based case
+ new QuorumControllerMetrics(KafkaYammerMetrics.defaultRegistry())
Review comment:
It doesn't look like there is a way to register default metrics in the Yammer metrics library.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on a change in pull request #11133: KAFKA-13140: KRaft brokers do not expose kafka.controller metrics
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #11133:
URL: https://github.com/apache/kafka/pull/11133#discussion_r685268642
##########
File path: core/src/main/scala/kafka/server/KafkaRaftServer.scala
##########
@@ -100,6 +101,9 @@ class KafkaRaftServer(
controllerQuorumVotersFuture
))
} else {
+ // we need to register the various kafka.controller metrics
+ // for backwards compatibility with the ZooKeeper-based case
+ new QuorumControllerMetrics(KafkaYammerMetrics.defaultRegistry())
Review comment:
> How about having a static method in QuorumControllerMetrics that does this and doesn't use gauges since they are not needed
Yeah, I don't see a way that this can be done. The registry requires that we add a Meter, Counter, Histogram, Timer, or Gauge. The Gauges don't take up a lot of resources anyway, so it doesn't seem like a big deal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #11133: KAFKA-13140: KRaft brokers do not expose kafka.controller metrics
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #11133:
URL: https://github.com/apache/kafka/pull/11133#issuecomment-904014137
I am not sure we should expose controller metrics for brokers that don't have the controller role.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a change in pull request #11133: KAFKA-13140: KRaft brokers do not expose kafka.controller metrics
Posted by GitBox <gi...@apache.org>.
jsancio commented on a change in pull request #11133:
URL: https://github.com/apache/kafka/pull/11133#discussion_r677612215
##########
File path: core/src/main/scala/kafka/server/KafkaRaftServer.scala
##########
@@ -100,6 +101,9 @@ class KafkaRaftServer(
controllerQuorumVotersFuture
))
} else {
+ // we need to register the various kafka.controller metrics
+ // for backwards compatibility with the ZooKeeper-based case
+ new QuorumControllerMetrics(KafkaYammerMetrics.defaultRegistry())
Review comment:
This works because the constructor of `QuorumControllerMetrics` registers those metrics against the Yammer registry. How about having a static method in `QuorumControllerMetrics` that does this and doesn't use guages since they are not needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #11133: KAFKA-13140: KRaft brokers do not expose kafka.controller metrics
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #11133:
URL: https://github.com/apache/kafka/pull/11133#issuecomment-968959242
Closing as [KIP-771: KRaft brokers without the "controller" role should not expose controller metrics](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=188743985) was adopted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org