[
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
