junrao commented on code in PR #18444:
URL: https://github.com/apache/kafka/pull/18444#discussion_r1915800593
##########
core/src/main/java/kafka/server/share/SharePartitionManager.java:
##########
@@ -693,6 +717,38 @@ private static void removeSharePartitionFromCache(
}
}
+ /**
+ * The handler to update the failed share fetch request metrics.
+ *
+ * @return A BiConsumer that updates the failed share fetch request
metrics.
+ */
+ private BiConsumer<Collection<TopicIdPartition>, Boolean>
failedShareFetchMetricsHandler() {
+ return (topicIdPartitions, allTopicPartitionsFailed) -> {
+ // Update failed share fetch request metric.
+ topicIdPartitions.forEach(topicIdPartition ->
+
brokerTopicStats.topicStats(topicIdPartition.topicPartition().topic()).failedShareFetchRequestRate().mark());
+ if (allTopicPartitionsFailed) {
Review Comment:
1. The intention of `brokerTopicStats.topicStats` and
`brokerTopicStats.allTopicsStats` is that they are recording the same value,
just at different level. The idea is that if it's too expensive to track at the
topic level, one can at least get the same signal at the aggregate level.
2. I agree that marking the request rate metric for each partition seems
incorrect. We need to mark the metric at most once per request and we want to
do that consistently for total and failed requests.
3. If it's important to track shareFetchRequests with all topic-partition
failed, we can introduce a separate metric for that.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
