----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19388/#review38261 -----------------------------------------------------------
Thanks for the patch. Some comments. 1. For producer level stats (ie., not node or topic level), for jmx beans, the package name becomes kafka.producer (since the bean name is kafka.producer:type=client-id). For the topic and node level stats, jmx beans have the package name kafka.producer.client-id. Perhaps we can name the producer level stats under kafka.producer.client-id.aggregate? 2. Could we make the metrics.timeWindowNs and metrics.samples configurable? 3. Also, in Metrics, what's the purpose of eventWindow? It's not being used and I am not sure if there is a case when we will roll the window by count, instead of size. 4. When testing using console producer, I sent 1 message and expected the topic level message rate to be non-zero for at least 30 secs (on average, I expect the non-zero value to last 45 secs). However, in multiple tests, the number drops to 0 well before 30 secs. 5. Metric: Our measurement returns a double. However, we don't synchronize btw the reader and the writer of the double. Since double access is not guaranteed to be atomic in java, we could see incorrect value in the output. 6. Not sure why we need to change the license header. Perhaps we can have a separate jira to standardize all headers if there is inconsistency? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/19388/#comment70316> Perhaps we could distinguish btw metadata request and produce request? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/19388/#comment70317> Why this method needs nowNs while the ones above don't? clients/src/main/java/org/apache/kafka/common/metrics/stats/SampledStat.java <https://reviews.apache.org/r/19388/#comment70315> When I ran a producer producing at a constant rate of 1 msg/sec, occasionally, I saw a blip of -infinity on metrics like max-message-size in jconsole. I think this can be caused by the following. When switching to a new window, we first reset the sample in the new window and then update the new value. If the jmx reader sneaks in btw, it would read a sample with no data in it. For metrics like max, this translates to -infinity, which will result in -infinity when combined with other samples. To fix this, I am wondering if we should introduce a method like resetAndUpdate() so that we can do the reset and update atomically? clients/src/main/java/org/apache/kafka/common/network/Selector.java <https://reviews.apache.org/r/19388/#comment70313> Could we distinguish btw metadata and produce requests? clients/src/main/java/org/apache/kafka/common/network/Selector.java <https://reviews.apache.org/r/19388/#comment70314> Could we distinguish btw metadata and produce responses? - Jun Rao On March 20, 2014, 12:30 a.m., Jay Kreps wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19388/ > ----------------------------------------------------------- > > (Updated March 20, 2014, 12:30 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1251 > https://issues.apache.org/jira/browse/KAFKA-1251 > > > Repository: kafka > > > Description > ------- > > KAFKA-1251: Add metrics to the producer. > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > 1ac69436f117800815b8d50f042e9e2a29364b43 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java > 33d62a4b83fbab5b22b91b22f6b744af1c98d262 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java > 673b2962771c28ceb3c7a6c0fd6f69521bd7ed16 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java > 038a05a94b795ec0a95b2d40a89222394b5a74c4 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > 565331dfb9cd1d65be37ed97830aa42e44d2e127 > > clients/src/main/java/org/apache/kafka/clients/tools/ProducerPerformance.java > 3ebbb804242be6a001b3bae6524afccc85a87602 > clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java > 6db2dfbe94c940efa37463298f0b0b1893e646e1 > clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java > 7e4849b7a148009c8a878349d7f0239108ccad8c > clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java > 3b0454f26490d1f4a2a80efb00165fc72587fbf8 > > clients/src/main/java/org/apache/kafka/common/metrics/stats/SampledStat.java > f8b413a8c273cdad56177fbc6971fece4feb86b3 > clients/src/main/java/org/apache/kafka/common/network/ByteBufferSend.java > 9305b61ddeaa2bb400cbbb6d3c99c8ecaade6b8f > clients/src/main/java/org/apache/kafka/common/network/NetworkReceive.java > 51d4892dfc18580e5e213d386c5de387a47d3c6b > clients/src/main/java/org/apache/kafka/common/network/Selector.java > 983963200ce81614577cd6182a5d2f10c22b95d4 > clients/src/test/java/org/apache/kafka/clients/producer/MetadataTest.java > 09a5355d25a3b94c8e23caa2adc77cb1c59368b9 > > clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java > ed5690641a22fbe4bd91b0c6055d465944b08c06 > clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java > 12c9500ce4387306ab5aa7a5781b4aca52b86604 > clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java > 90e2dcf5434db546387302fb0219edfdb363592e > clients/src/test/java/org/apache/kafka/test/MetricsBench.java > 7239b4a56e93f019e66aa2cf2aa9b04c26908bfd > > Diff: https://reviews.apache.org/r/19388/diff/ > > > Testing > ------- > > > Thanks, > > Jay Kreps > >
