[jira] [Commented] (KAFKA-622) Create mbeans per client

["Neha Narkhede (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/KAFKA-622?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13499861#comment-13499861 ] 

Neha Narkhede commented on KAFKA-622:
-------------------------------------

1. AbstractFetcherThread
1.1 Maybe the clientid validation could happen once on process startup (while instantiating the config) instead of on every fetcher thread. In other words, would it make sense to let the broker startup
 if the config doesn't comply with the allowed values ?
1.2 How about renaming FetcherLagStat to FetcherLagStats, same for FetcherStat and ConsumerTopicStat and any APIs that are get*Stat.
1.3 The change to FetcherLagStat.getFetcherLagMetrics serves the purpose but seems slightly hacky. Instead of changing the name of the topic by prepending the clientid to it, what do you think about ch
anging FetcherLagMetrics take in clientId, (topic, partition) ? The valueFactory can be changed to pass in clientid to the constructor of FetcherLagMetrics.
1.4 In other parts of code, we've been trying to move away from using a tuple of topic, partition as the key to a map and using TopicPartition instead. The reason being its unsafe since scala doesn't do a good job of doing the right thing on map/fold operations on such a map. Wondering if you were up for making that change in this patch ? If not, then lets file another JIRA to fix that.

2. ConsumerTopicStat.scala
2.1 Change the name to ConsumerTopicStats
2.2 Why does clientId default to empty string ?
2.3 Same change as 1.3 to ConsumerTopicMetrics
2.4 I wonder if it makes sense to allow creating ConsumerIterator without passing in ConsumerTopicStat ? ConsumerTopicStat is instantiated in ZookeeperConsumerConnector on startup. The same object need
s to be passed around everywhere else in the consumer. However, the code is allowing ConsumerTopicStat to be re-instantiated in ConsumerIterator with an empty clientId. Isn't that going to create new m
etrics objects tracking different values for the same class of metrics ? I guess the right thing is to not provide it a default

3. DefaultEventHandler
3.1 Same as 2.4 here for ProducerStats and ProducerTopicStat. 3.2 Let's standardize the naming here. One is ProducerStats and the other is ProducerTopicStat. How about renaming ProducerTopicStat to ProducerTopicStats ?

4. PartitionTopicInfo
Same as 2.4 here 

5. Producer 
5.1 What is the point of passing in empty string to ProducerTopicStat ? If that is testing only, how about passing in a clientid like "test-producer". I also think it is better to not change the constr
uctutor in this way and instead have the test pass in the producerStats object.
5.2 The clientId is validated after it is passed in to ProducerSendThread. Let's move the clientId validation all the way up in the constructor.
5.3 Rename getProducer*Stat to getProducer*Stats 
5.4 Same as 1.3 here as well
5.5 Having a separate AsyncProducerStats object seems a little clunky and is probably a remnant from the time when we explicitly exposed AsyncProducer and SyncProducer as public APIs. I'm guessing movi
ng this to ProducerStats would be better. That way you have only 2 metrics to worry about on the producer, one is ProducerStats and the other is per-topic stats exposed through ProducerTopicStats
 
6. ProducerPool
6.1 Same as 5.1 here, let's not change the constructor of ProducerPool in this manner, its ugly 

7. ProducerSendThread 
7.1 Let's not allow empty clientId as the default value. It should *always* be passed in correctly.
 
8. SimpleConsumer
8.1 I wonder what is the value of letting one instantiate a FetchRequestAndResponseStat with an empty clientId ?
8.2 Instead of adding a stat object to the constructor of SimpleConsumer, let's just add the clientId. It seems unnatural to have the user of SimpleConsumer to know how to instantiate the stat object. 
On the other hand, the purpose of clientId might be much easier to explain and understand.
8.3 Let's not change the static SimpleConsumer APIs to have the user pass in a stat object. These are fire and forget queries. At best, let's have the user pass in clientId or default it to be the host
 name or something meaningful but simple. ZookeeperConsumerConnector will pass in the right value for clientId

9. SyncProducer
9.1 Same as 8.2. Let's not change the constructor in this manner
9.2 Rename ProducerRequestStat to ProducerRequestStats

10. Is TopicTest deleted on purpose ?

                
> Create mbeans per client 
> -------------------------
>
>                 Key: KAFKA-622
>                 URL: https://issues.apache.org/jira/browse/KAFKA-622
>             Project: Kafka
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 0.8
>            Reporter: Swapnil Ghike
>            Assignee: Swapnil Ghike
>            Priority: Blocker
>              Labels: bugs, improvement
>             Fix For: 0.8
>
>         Attachments: kafka-622-v1.patch
>
>
> Currently we create one mbean of each type for a given mbean server, regardless of the number of clients. We should create MBeans per client for both producer and consumer. To do that we need to introduce clientId in mbean names.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira