----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19388/#review37843 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java <https://reviews.apache.org/r/19388/#comment69726> We seem to have 2 error sensors and both convey almost the same thing - the rate of errors. Is there a way to merge these into just one error-rate sensor? clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java <https://reviews.apache.org/r/19388/#comment69725> doc bug. clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java <https://reviews.apache.org/r/19388/#comment69722> maybe I'm missing something but shouldn't this be one of the Sender metrics and get updated in run() after this - // get the list of partitions with data ready to send List<TopicPartition> ready = this.accumulator.ready(now); clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java <https://reviews.apache.org/r/19388/#comment69721> Not part of this patch, but can we fix the doc to replace errorCode with exception? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/19388/#comment69709> I'm wondering why we pass in the latest time (time.milliseconds()) only to handleResponses() and handleDisconnects() when we pass in 'now' everywhere else in run()? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/19388/#comment69700> Is it required to pass in 'now'? Could it just be computed inside handleResponses just like 'ns'? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/19388/#comment69699> Should we also add an Avg metric for queue-time (queue-time-avg) similar to the one we have for batch-size (batch-size-avg)? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/19388/#comment69603> shouldn't we also add a message-error-rate metric to the errorSensor? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/19388/#comment69696> Can we collapse these 2 statements into simply for(InFlightRequest request : requests){} since it seems that we don't use the for loop index elsewhere other than getting the request from the list? clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java <https://reviews.apache.org/r/19388/#comment69723> typo: ellapsed -> elapsed Same in ProducerPerformance.java clients/src/main/java/org/apache/kafka/common/network/NetworkReceive.java <https://reviews.apache.org/r/19388/#comment69724> I wonder why your patch seems to change the formatting of the license header. Is that some eclipse setting? Maybe we should all use the same setting? clients/src/main/java/org/apache/kafka/common/network/Selector.java <https://reviews.apache.org/r/19388/#comment69718> It seems like it would suffice to know select-time-avg-ns and select-percentage. (similar to io-time-avg-ns and io-percentage). Probably we can get rid of select-calls-per-second since it doesn't seem actionable? Or do you see it as one of those metrics we would want to enable on-the-fly in the debug mode? - Neha Narkhede 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 > >
