[
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)
