> On Aug. 1, 2014, 11:33 p.m., Guozhang Wang wrote: > > To be more precisely, in the old clients we have four stats classes: > > > > 1. ProducerTopicStats > > 2. ProducerRequestStats > > 3. ProducerStats > > 4. FetchRequestAndResponseStats > > 5. ConsumerTopicStats > > > > Among them: > > > > 1) is accessed by Producer and DefaultEventHandler > > 2) is accessed by SyncProducer > > 3) is accessed by DefaultEventHandler > > 4) is accessed by SimpleConsumer > > 5) is accessed by ConsumerIterator, PartitionTopicInfo, and > > ZookeeperConsumerConnector > > > > All of the above modules can have or already have a close() call. Since for > > all of the metrics, we create new instances through client-ids. And hence, > > upon closing all these above modules we can just pass in the client id > > string to the Stats factories to close the metrics, and by doing so we do > > not need to keep track of the metrics owner.
Thank you very much for the review, Guozhang. The reason I passed in the metricOwner is because in this approach I have a global metric registry in KafkaMetricsGroup. That means whennever a metric is created with newGauge(),newMeter(),newTimer(),newHistogram(), a metric owner has to be specified. The same metric owner is needed for deregister the metric from yammer. In those five stats, they do the following two things: 1. create a kafka metric object(e.g. ConsumerTopicsMetrics) and put them into their own registry. 2. create yammer metrics and register them to yammer. While the kafka metrics can be removed easily with a client-id, deleting the yammer metrics needs more name translation. It could also potentially make the interaction with KafkaMetricsGroup less unified as there are many other stand alone metrics not using the stats factory. Jun also pointed out another issue that the current approach with a global yammer metrics map has insufficient isolation between objects and is somewhat hacky. Instead of a global metric map, he suggested to create a single metric registry for each instance and deregister the metrics in the registry when the instance exits. Without a global registry, maybe the metric owner is no longer needed to be passed in. Thanks! - Jiangjie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24196/#review49401 ----------------------------------------------------------- On Aug. 1, 2014, 9:26 p.m., Jiangjie Qin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24196/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2014, 9:26 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1567 > https://issues.apache.org/jira/browse/KAFKA-1567 > > > Repository: kafka > > > Description > ------- > > fix for metric memory leaking issue. > > > Diffs > ----- > > core/src/main/scala/kafka/client/ClientUtils.scala > ce7ede3f6d60e756e252257bd8c6fedc21f21e1c > core/src/main/scala/kafka/consumer/ConsumerFetcherManager.scala > b9e2bea7b442a19bcebd1b350d39541a8c9dd068 > core/src/main/scala/kafka/consumer/ConsumerIterator.scala > ac491b4da2583ef7227c67f5b8bc0fd731d705c3 > core/src/main/scala/kafka/consumer/ConsumerTopicStats.scala > ff5f470f7aa304917d3295fcb7702291ce7fe0b5 > core/src/main/scala/kafka/consumer/FetchRequestAndResponseStats.scala > 875eeeb73cba5bd034349f5e7b6e16dfdf544254 > core/src/main/scala/kafka/consumer/KafkaStream.scala > 805e91677034edcca74c74dab01a6d6d437ae442 > core/src/main/scala/kafka/consumer/PartitionTopicInfo.scala > 9c779ce072fb0a3426e96fd77f383cce6991a0f9 > core/src/main/scala/kafka/consumer/SimpleConsumer.scala > 0e64632210385ef63c2ad3445b55ac4f37a63df2 > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala > 65f518d47c7555c42c4bff39c211814831f4b8b6 > core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala > a20ab90165cc7ebb1cf44078efe23a53938c8df6 > core/src/main/scala/kafka/producer/Producer.scala > 4798481d573bbdce0ba39035c50f4c4411ad0469 > core/src/main/scala/kafka/producer/ProducerRequestStats.scala > 96942205a6a461e122e003add1ab9bcebde1fe16 > core/src/main/scala/kafka/producer/ProducerStats.scala > e1610d3c602fb0f5f4cc237cb8b4e0d168a41530 > core/src/main/scala/kafka/producer/ProducerTopicStats.scala > ed209f4773dedb09e9a34005e6849730229aa6e9 > core/src/main/scala/kafka/producer/SyncProducer.scala > 489f0077512d9a69be81649c490274964290fa40 > core/src/main/scala/kafka/producer/async/DefaultEventHandler.scala > d8ac915de31a26d7aa67760d69373975cacd0c9d > core/src/main/scala/kafka/producer/async/ProducerSendThread.scala > 42e9c741c2dcef756416832f11d37678cb7710ee > core/src/main/scala/kafka/server/AbstractFetcherManager.scala > 9390edf37dae9815d4a3790fdd31dac422382698 > core/src/main/scala/kafka/server/AbstractFetcherThread.scala > 3b15254f32252cf824d7a292889ac7662d73ada1 > gradle.properties 4827769a3f8e34f0fe7e783eb58e44d4db04859b > > Diff: https://reviews.apache.org/r/24196/diff/ > > > Testing > ------- > > > Thanks, > > Jiangjie Qin > >
