[
https://issues.apache.org/jira/browse/KAFKA-622?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13502889#comment-13502889
]
Neha Narkhede commented on KAFKA-622:
-------------------------------------
Thanks for incorporating suggestions in the first review!
1. AbstractFetcherThread.scala
FetcherLagMetrics now takes in a nested tuples. Scala's support for tuples
makes it easier to express short lived data structures that need to be paired
conveniently, but they tend to reduce code readability. In addition to this,
we've been bitten by incomplete Scala semantics when comparing two tuples or
using them for some of the functional programming collections APIs. Over time,
as we understood this more, we now encourage use of tuples only for expressing
short lived objects within maybe the same API. We haven't changed all the past
usages to conform to this yet, but we do hope we can avoid this in the future
usage. Having said that, I think here it is a good idea to change this to
accept two arguments - String and TopicAndPartition.
2. ClientUtils
Both producers and consumers use this API to fetch metadata and it seems like
we have hardcoded the clientId. The clientId to create sync producer should be
the one that the consumer/producer is instantiated with.
3. Producer.scala
3.1 I think you are validating the clientId twice in the constructor that takes
in just the ProducerConfig.
3.2 Maybe this was done for convenience, what is the reason that the
constructor argument to ProducerTopicMetrics is a tuple vs 2 separate strings ?
3.3 We definitely want to avoid this -
private val stats = new Pool[(String, String),
ProducerTopicMetrics](Some(valueFactory))
Change this to use a case class TopicAndClientId here as well as
ConsumerTopicStats.scala.
4. ProducerPool.scala
createSyncProducer is overloaded to create 2 very similar but confusing
methods. One takes Broker as the second argument, the other takes it as the
first argument. I see why you need two, but it will be great to add some docs
to explain where each of those is used.
5. ProducerSendThread.scala
There needs to be a delimiter ("-") between the clientId and ProducerQueueSize
6. ProducerConfig
Not sure which patch introduced this property, but it is inconsistent with the
property naming conventions. It happens to be the only one that uses an
underscore in the property name. Besides, the one in the consumer is called
just clientid. Let's please change this to clientid.
val clientId =
props.getString("producer.request.client_id",SyncProducerConfig.DefaultClientId)
7. ZookeeperConsumerConnector
It will be good to have some summary stats here that aggregates from individual
simple consumers. Often users wouldn't want to do this math themselves.
8. What happened to ConsumerTopicStats ?
> 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, kafka-622-v2.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
