[
https://issues.apache.org/jira/browse/KAFKA-1481?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14173157#comment-14173157
]
Jun Rao commented on KAFKA-1481:
--------------------------------
Thanks for the patch. The IDEA one applies successfully for me. However, it
seems to cause compilation failure since in a few cases, two lines are
incorrectly merged into a single one. Do you think you can try our patch review
tool
(https://cwiki.apache.org/confluence/display/KAFKA/Patch+submission+and+review)?
Some comments.
1. KafkaMetricsGroup:
1.1 We now have 2
new MetricName("kafka.consumer", "ConsumerTopicMetrics",
"MessagesPerSec"),
and 2
new MetricName("kafka.consumer", "ConsumerTopicMetrics", "BytesPerSec"),
1.2 Could we combine the following two methods
def newGauge[T](name: String, mBeanName: String, metric: Gauge[T])
def newGauge[T](name: String, metric: Gauge[T]) : Gauge[T]
to
def newGauge[T](name: String, metric: Gauge[T], tags: Map[String, String] =
Map.empty)? We can then general the metric name based on name and the tags.
This will consolidate the metric name generation in a single place. In this
patch, we have too many places generating the key/value string for the metric
name.
2. AbstractFetcherThread:
2.1 name in this class should be just used for the thread name and it shouldn't
be included in the metric name.
2.2. The way that we get the metric name in classes like ClientIdAndBroker,
ClientIdBrokerTopicPartition and ClientIdAndTopic is not consistently.
Sometimes, we generate the key/value in the toString(). Some other times, we
rely on the key/value string to be passed in. It's easier to understand if
those classes are constructed by just passing in the clientId string, the
broker object, the topic string and the partition number, etc. We can generate
the metric name by using what's described in 1.2.
2.3 FetcherLagStats just needs to take clientId, instead of ClientIdAndBroker
since the per partition lag metric is unique per client id.
2.4 The metric name for a Broker can just use the brokerId, instead of its host
and port.
3. Throttler: Do we need to add mBeanName to the constructor? It seems that
nobody is using it at the moment.
4. KafkaMetricsGroup: We need to change the way how removeAllMetricsInList()
works. We need to do equal comparison on group, type and name and do regex
matching with clientId on MetricName.mBeanName.
> 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.2
>
> 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_IDEA_IDE_2014-10-14_21-53-35.patch,
> KAFKA-1481_IDEA_IDE_2014-10-15_10-23-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)
