[
https://issues.apache.org/jira/browse/KAFKA-1481?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14179102#comment-14179102
]
Jun Rao commented on KAFKA-1481:
--------------------------------
Thanks for the patch. Some comments below.
20. KafkaMetricsGroup:
20.1 In the following, instead of doing map(kv => ), could we do map {
case(key, value) => }?
.filter(_._2 != "").map(kv => "%s=%s".format(kv._1, kv._2))
20.2 In the following, shouldn't the pattern be (".*" + "clientId=" + clientId
+ ".*").r
val pattern = (".*" + clientId + ".*").r
21. TaggableInfo: tags should be an immutable hashmap, right?
case class TaggableInfo(tags: mutable.LinkedHashMap[String, String]) extends
Taggable {
22. New files:
22.1 The license header should be at the very beginning, before the package and
import.
22.2 I am not sure if we really need to have the Taggable trait. For example,
in ConsumerTopicMetrics, we can just take the original clientIdAndTopic and
explicitly create the tag for clientId and topic when creating those metrics.
The tags are really specific to the metrics. So, we probably don't need to
expose them in places other than where the metrics are created.
23. ReplicaFetcherManager: The first statement in shutdown() is on the same
line as the method. A similar issue happens in a few other classes like
RequestChannel and ConsumerFetcherManager. Perhaps you can follow the patch
creation process in
https://cwiki.apache.org/confluence/display/KAFKA/Patch+submission+and+review#Patchsubmissionandreview-Simplecontributorworkflow
24.
FetchRequestAndResponseStatsRegistry.removeConsumerFetchRequestAndResponseStats():
Should we use "clientId=" + clientId in pattern?
25. SimpleConsumer: Ideally, we shouldn't change the constructor since this
will be an api change.
26. Could you add a unit test to make sure that after a producer/consumer is
closed, all metrics specific to the producer/consumer are removed?
> 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_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)
