[ https://issues.apache.org/jira/browse/KAFKA-1481?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14195545#comment-14195545 ]
Jun Rao commented on KAFKA-1481: -------------------------------- Thanks for the patch. Just some minor comments below. Otherwise, +1 from me. 60. AbstractFetcherManager: In the following, we don't need to wrap new Gauge in {}. "MinFetchRate", { new Gauge[Double] { // current min fetch rate across all fetchers/topics/partitions def value = { val headRate: Double = fetcherThreadMap.headOption.map(_._2.fetcherStats.requestRate.oneMinuteRate).getOrElse(0) fetcherThreadMap.foldLeft(headRate)((curMinAll, fetcherThreadMapEntry) => { fetcherThreadMapEntry._2.fetcherStats.requestRate.oneMinuteRate.min(curMinAll) }) } } }, 61. AbstractFetcherThread: Could we align "topic" and "partition" to the same column as "clientId"? Ditto in a few other places. Map("clientId" -> metricId.clientId, "topic" -> metricId.topic, "partition" -> metricId.partitionId.toString) ) 62. FetchRequestAndResponseMetrics: In the following, could we put Map in a separate line? val tags = metricId match { case ClientIdAndBroker(clientId, brokerHost, brokerPort) => Map("clientId" -> clientId, "brokerHost" -> brokerHost, "brokerPort" -> brokerPort.toString) case ClientIdAllBrokers(clientId) => Map("clientId" -> clientId, "allBrokers" -> "true") } 62. TestUtils: It's a bit weird that sendMessagesToBrokerPartition() has both a config and configs. We actually don't need the config any more. When sending a message to a partition, we actually don't know which broker the partition is on. So, the message can just be of the form test + "-" + partition + "-" + x. We can also rename the method to sendMessagesToPartition(). > Stop using dashes AND underscores as separators in MBean names > -------------------------------------------------------------- > > Key: KAFKA-1481 > URL: https://issues.apache.org/jira/browse/KAFKA-1481 > Project: Kafka > Issue Type: Bug > Components: core > Affects Versions: 0.8.1.1 > Reporter: Otis Gospodnetic > Priority: Critical > Labels: patch > Fix For: 0.8.3 > > Attachments: KAFKA-1481_2014-06-06_13-06-35.patch, > KAFKA-1481_2014-10-13_18-23-35.patch, KAFKA-1481_2014-10-14_21-53-35.patch, > KAFKA-1481_2014-10-15_10-23-35.patch, KAFKA-1481_2014-10-20_23-14-35.patch, > KAFKA-1481_2014-10-21_09-14-35.patch, KAFKA-1481_2014-10-30_21-35-43.patch, > KAFKA-1481_2014-10-31_14-35-43.patch, > KAFKA-1481_2014-11-03_16-39-41_doc.patch, > KAFKA-1481_2014-11-03_17-02-23.patch, > KAFKA-1481_IDEA_IDE_2014-10-14_21-53-35.patch, > KAFKA-1481_IDEA_IDE_2014-10-15_10-23-35.patch, > KAFKA-1481_IDEA_IDE_2014-10-20_20-14-35.patch, > KAFKA-1481_IDEA_IDE_2014-10-20_23-14-35.patch > > > MBeans should not use dashes or underscores as separators because these > characters are allowed in hostnames, topics, group and consumer IDs, etc., > and these are embedded in MBeans names making it impossible to parse out > individual bits from MBeans. > Perhaps a pipe character should be used to avoid the conflict. > This looks like a major blocker because it means nobody can write Kafka 0.8.x > monitoring tools unless they are doing it for themselves AND do not use > dashes AND do not use underscores. > See: http://search-hadoop.com/m/4TaT4lonIW -- This message was sent by Atlassian JIRA (v6.3.4#6332)