[
https://issues.apache.org/jira/browse/KAFKA-646?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Swapnil Ghike updated KAFKA-646:
--------------------------------
Attachment: kafka-646-patch-num1-v1.patch
This patch has a bunch of refactoring changes and a couple of new additions.
Addressing Jun's comments:
These are all great catches! Thanks for being so thorough.
60. By default, metrics-core will return an existing metric object of the same
name using a getOrCreate() like functionality. As discussed offline, we should
fail the clients that use an already registered clientId name. We will need to
create two objects thaty contain hashmaps to record the existing producer and
consumer clientIds and methods to throw an exception if a client attempts to
use an existing clientId. I worked on this change a bit, but it breaks a lot of
our unit tests (about half) and the refactoring will take some time. Hence, I
think it will be better if I submit a patch for all other changes and create
another patch for this issue under this jira. Until then we can keep this jira
open.
61. For recording stats about all topics, I am now using a string "All.Topics".
Since '.' is not allowed in the legal character set for topic names, this will
differentiate from a topic named AllTopics.
62. Yes, we should validate groupId. Added the functionality and a unit test.
It has the same validation rules as ClientId.
63. A metric name is something like (clientId + topic + some string) and this
entire string is limited by fillename size. We already allow topic name to be
at most 255 bytes long. We could fix max lengths for each of clientId, groupId,
topic name so that the metric name never exceeds filename size. But those
lengths will be quite arbitrary, perhaps we should skip the check on the length
of clientId and groupId.
64. Removed brokerInfo from the clientId used to instantiate
FetchRequestBuilder.
Refactoring:
1. Moved validation of clientId at the end of instantiation of ProducerConfig
and ConsumerConfig.
- Created static objects ProducerConfig and ConsumerConfig which contain a
validate() method.
2. Created global *Registry objects in which each high level Producer and
Consumer can register their *stats objects.
- These objects are registered in the static object only once using
utils.Pool.getAndMaybePut functionality.
- This will remove the need to pass *stats objects around the code in
constructors (I thought having the metrics objects right up in the constructors
was a bit intrusive, since one doesn't quite always think about the monitoring
mechanism while instantiating various modules of the program, for example while
unit testing.)
- Instead of the constructor, each concerned class obtains the *Stats objects
from the global registry object.
- This cleans up any metrics objects created in the unit tests.
- Special mention: The producer constructors are back to the old themselves.
With clientId validation moved to *Config objects, the intermediate Producer
constructor that merely separated the parameters of a quadruplet is gone.
3. Created separate files
- for ProducerStats, ProducerTopicStats, ProducerRequestStats in
kafka.producer package and for FetchRequestAndResponseStats in kafka.consumer
package. Thought it was appropriate given that we already had
ConsumerTopicStats in a separate file, and since the code for metrics had
increased in size due to addition of *Registry and Aggregated* objects. Added
comments.
- for objects Topic, ClientId and GroupId in kafka.utils package.
- to move the helper case classes ClientIdAndTopic, ClientIdAndBroker to
kafka.common package.
4. Renamed a few variables to easier names (anyOldName to "metricId" change).
New additions:
1. Added two objects to aggregate metrics recorded by SyncProducers and
SimpleConsumers at the high level Producer and Consumer.
- For this, changed KafkaTimer to accept a list of Timers. Typically we will
pass a specificTimer and a globalTimer to this KafkaTimer class. Created a new
KafkaHistogram in a similar way.
2. Validation of groupId.
Issues:
1. Initializing the aggregator metrics with default values: For example, let's
say that a syncProducer could be created (which will register a
ProducerRequestStats mbean for this syncProducer). However, if no request is
sent by this syncProducer then the absense of its data is not reflected in the
aggregator histogram. For instance, the min requestSize for the syncProducer
that never sent a request will be 0, but this won't be accurately represented
in the aggregator histogram. Thus, we need to understand that if the request
count of a syncProducer is 0, then its data will not be accurately reflected in
the aggregator histogram.
The question is whether it is possible to inform the aggregator histogram of
some default values without increasing the request count of any syncProducer or
the aggregated stats.
Further proposed changes:
Another patch under this jira to address comment 60 by Jun.
> Provide aggregate stats at the high level Producer and
> ZookeeperConsumerConnector level
> ---------------------------------------------------------------------------------------
>
> Key: KAFKA-646
> URL: https://issues.apache.org/jira/browse/KAFKA-646
> Project: Kafka
> Issue Type: Bug
> Affects Versions: 0.8
> Reporter: Swapnil Ghike
> Assignee: Swapnil Ghike
> Priority: Blocker
> Labels: bugs
> Fix For: 0.8
>
> Attachments: kafka-646-patch-num1-v1.patch
>
>
> WIth KAFKA-622, we measure ProducerRequestStats and
> FetchRequestAndResponseStats at the SyncProducer and SimpleConsumer level
> respectively. We could also aggregate them in the high level Producer and
> ZookeeperConsumerConnector level to provide an overall sense of
> request/response rate/size at the client level. Currently, I am not
> completely clear about the math that might be necessary for such aggregation
> or if metrics already provides an API for aggregating stats of the same type.
> We should also address the comments by Jun at KAFKA-622, I am copy pasting
> them here:
> 60. What happens if have 2 instances of Consumers with the same clientid in
> the same jvm? Does one of them fail because it fails to register metrics?
> Ditto for Producers.
> 61. ConsumerTopicStats: What if a topic is named AllTopics? We use to handle
> this by adding a - in topic specific stats.
> 62. ZookeeperConsumerConnector: Do we need to validate groupid?
> 63. ClientId: Does the clientid length need to be different from topic length?
> 64. AbstractFetcherThread: When building a fetch request, do we need to pass
> in brokerInfo as part of the client id? BrokerInfo contains the source broker
> info and the fetch requests are always made to the source broker.
--
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
