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/11/07 07:11:14 UTC
[GitHub] [kafka] dongjinleekr opened a new pull request #11474: KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics
dongjinleekr opened a new pull request #11474:
URL: https://github.com/apache/kafka/pull/11474
I inspected this issue a little bit and found the following:
1. There are 18 metrics in `kafka.server:type=BrokerTopicMetrics` and 4 of them should be counted per request, but actually counted per `TopicPartition`:
- `kafka.server:type=BrokerTopicMetrics,name=TotalProduceRequestsPerSec`
- `kafka.server:type=BrokerTopicMetrics,name=TotalFetchRequestsPerSec`
- `kafka.server:type=BrokerTopicMetrics,name=FailedProduceRequestsPerSec`
- `kafka.server:type=BrokerTopicMetrics,name=FailedFetchRequestsPerSec`
2. 5 of them are omitted in the documentation (see: #11473)
- `kafka.server:type=BrokerTopicMetrics,name=TotalProduceRequestsPerSec`
- `kafka.server:type=BrokerTopicMetrics,name=TotalFetchRequestsPerSec`
- `kafka.server:type=BrokerTopicMetrics,name=FailedProduceRequestsPerSec`
- `kafka.server:type=BrokerTopicMetrics,name=FailedFetchRequestsPerSec`
- `kafka.server:type=BrokerTopicMetrics,name=BytesRejectedPerSec`
### 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] splett2 commented on a change in pull request #11474: KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics
Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11474:
URL: https://github.com/apache/kafka/pull/11474#discussion_r744934377
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -953,6 +957,16 @@ class ReplicaManager(val config: KafkaConfig,
}
}
}
+
+ requestedTopicSet.foreach { case topic =>
+ brokerTopicStats.topicStats(topic).totalProduceRequestRate.mark()
+ }
Review comment:
nit: maybe something like
`requestedTopics.map(brokerTopicStats.topicStats).foreach(_.totalProduceRequestRate.mark())`
flows a bit better.
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -894,9 +894,11 @@ class ReplicaManager(val config: KafkaConfig,
requiredAcks: Short,
requestLocal: RequestLocal): Map[TopicPartition, LogAppendResult] = {
val traceEnabled = isTraceEnabled
+ val failedTopicSet = mutable.Set[String]()
def processFailedRecord(topicPartition: TopicPartition, t: Throwable) = {
val logStartOffset = onlinePartition(topicPartition).map(_.logStartOffset).getOrElse(-1L)
- brokerTopicStats.topicStats(topicPartition.topic).failedProduceRequestRate.mark()
+ // Every time appending to a local log fails, collect the topic names into the set.
+ failedTopicSet.add(topicPartition.topic)
brokerTopicStats.allTopicsStats.failedProduceRequestRate.mark()
Review comment:
we would want this metric to be incremented once per request instead of once per TP.
ditto for `totalProduceRequestRate`, `totalFetchRequestRate`, `failedFetchRequestRate`
Otherwise the `allTopicsStats` will be inconsistent with the sum of individual topic stats.
--
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] splett2 commented on a change in pull request #11474: KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics
Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11474:
URL: https://github.com/apache/kafka/pull/11474#discussion_r745132675
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -953,6 +957,16 @@ class ReplicaManager(val config: KafkaConfig,
}
}
}
+
+ requestedTopicSet.foreach { case topic =>
+ brokerTopicStats.topicStats(topic).totalProduceRequestRate.mark()
+ }
Review comment:
this is a good point, although I would imagine that the set of topics per request is usually small. In that case it may be fine to keep the update code as is. Maybe change:
`case topic =>` to just `topic =>` as i dont think the case is necessary for iterating over a set.
--
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] dongjinleekr commented on pull request #11474: KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #11474:
URL: https://github.com/apache/kafka/pull/11474#issuecomment-962563047
@splett2 Is this approach the same thing you thought?
--
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] dongjinleekr commented on a change in pull request #11474: KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11474:
URL: https://github.com/apache/kafka/pull/11474#discussion_r745139046
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -953,6 +957,16 @@ class ReplicaManager(val config: KafkaConfig,
}
}
}
+
+ requestedTopicSet.foreach { case topic =>
+ brokerTopicStats.topicStats(topic).totalProduceRequestRate.mark()
+ }
Review comment:
all // in short, just replace `case topic =>` to `topic =>`; do I understand correctly?
--
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] dajac commented on a change in pull request #11474: KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11474:
URL: https://github.com/apache/kafka/pull/11474#discussion_r745092766
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -953,6 +957,16 @@ class ReplicaManager(val config: KafkaConfig,
}
}
}
+
+ requestedTopicSet.foreach { case topic =>
+ brokerTopicStats.topicStats(topic).totalProduceRequestRate.mark()
+ }
Review comment:
The downside of using 'map' is that it will create an intermediate collection.
--
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] splett2 commented on a change in pull request #11474: KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics
Posted by GitBox <gi...@apache.org>.
splett2 commented on a change in pull request #11474:
URL: https://github.com/apache/kafka/pull/11474#discussion_r745132675
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -953,6 +957,16 @@ class ReplicaManager(val config: KafkaConfig,
}
}
}
+
+ requestedTopicSet.foreach { case topic =>
+ brokerTopicStats.topicStats(topic).totalProduceRequestRate.mark()
+ }
Review comment:
this is a good point. In that case it may be fine to keep the update code as is. Maybe change:
`case topic =>` to just `topic =>` as i dont think the case is necessary for iterating over a set.
--
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] dongjinleekr commented on a change in pull request #11474: KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11474:
URL: https://github.com/apache/kafka/pull/11474#discussion_r745086781
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -894,9 +894,11 @@ class ReplicaManager(val config: KafkaConfig,
requiredAcks: Short,
requestLocal: RequestLocal): Map[TopicPartition, LogAppendResult] = {
val traceEnabled = isTraceEnabled
+ val failedTopicSet = mutable.Set[String]()
def processFailedRecord(topicPartition: TopicPartition, t: Throwable) = {
val logStartOffset = onlinePartition(topicPartition).map(_.logStartOffset).getOrElse(-1L)
- brokerTopicStats.topicStats(topicPartition.topic).failedProduceRequestRate.mark()
+ // Every time appending to a local log fails, collect the topic names into the set.
+ failedTopicSet.add(topicPartition.topic)
brokerTopicStats.allTopicsStats.failedProduceRequestRate.mark()
Review comment:
(After some thinking) Oh, you are right. I omitted them. :sweat:
--
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] dongjinleekr commented on pull request #11474: KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #11474:
URL: https://github.com/apache/kafka/pull/11474#issuecomment-967161641
@dajac @splett2 Here is the update. I updated/added some tests to check whether the metrics work correctly.
Add to this, I found that some methods and variables in `ReplicaManagerTest` are not separated per test, like the topic name `foo`. As soon as this PR is merged, I will open an additional PR for improving it.
--
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