> On March 20, 2014, 6:02 p.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java,
> > line 133
> > <https://reviews.apache.org/r/19388/diff/4/?file=528763#file528763line133>
> >
> > 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?
This is a bug. Both sensors should have been called "errors" (i.e. they should
have been the same). The key thing is that you can say
this.metrics.sensor("errors")
in two parts of the code and get the same sensor. This is important for cases
like this where the errors occur in totally different places.
> On March 20, 2014, 6:02 p.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java,
> > line 114
> > <https://reviews.apache.org/r/19388/diff/4/?file=528765#file528765line114>
> >
> > 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);
> >
It can work either way. That style just directly exposes that method as a
metric. We could alternately record each check and take the running average as
you propose.
> On March 20, 2014, 6:02 p.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java,
> > line 232
> > <https://reviews.apache.org/r/19388/diff/4/?file=528767#file528767line232>
> >
> > 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()?
Dunno, clearly the product of a diseased mind. Fixed that.
> On March 20, 2014, 6:02 p.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java,
> > line 424
> > <https://reviews.apache.org/r/19388/diff/4/?file=528767#file528767line424>
> >
> > Is it required to pass in 'now'? Could it just be computed inside
> > handleResponses just like 'ns'?
It's not required. The goal was (1) to make the full execution of a select loop
occur with a single unique timestamp, and (2) avoid checking the clock 12 times
since it is expensive.
> On March 20, 2014, 6:02 p.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java,
> > line 787
> > <https://reviews.apache.org/r/19388/diff/4/?file=528767#file528767line787>
> >
> > 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)?
Wups, missed that. Added
"message-queue-time-avg" => "The average time in ms record batches spent in
the record accumulator.",
"message-queue-time-max" => "The maximum time in ms record batches spent in
the record accumulator.",
> On March 20, 2014, 6:02 p.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java,
> > line 794
> > <https://reviews.apache.org/r/19388/diff/4/?file=528767#file528767line794>
> >
> > shouldn't we also add a message-error-rate metric to the errorSensor?
Yes, nice catch.
> On March 20, 2014, 6:02 p.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java,
> > line 814
> > <https://reviews.apache.org/r/19388/diff/4/?file=528767#file528767line814>
> >
> > 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?
I try to avoid that style of for loop for lists as it ends up creating an
iterator object. Maybe that is crazy?
> On March 20, 2014, 6:02 p.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/network/NetworkReceive.java,
> > line 6
> > <https://reviews.apache.org/r/19388/diff/4/?file=528774#file528774line6>
> >
> > 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?
We should. The code had a consistent line width of 120. The patch that added
licenses had them at 80, though. We should standardize because having it done
automatically leads to much much better formatting of comments.
> On March 20, 2014, 6:02 p.m., Neha Narkhede wrote:
> > clients/src/main/java/org/apache/kafka/common/network/Selector.java, line
> > 423
> > <https://reviews.apache.org/r/19388/diff/4/?file=528775#file528775line423>
> >
> > 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?
I do see that as useful for debugging if you are trying to figure out what the
selector thread is doing. It is a little bit of an implementation detail but
"iterations-per-second" is the basic performance metric for the I/O thread
other than bytes-in/out.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19388/#review37843
-----------------------------------------------------------
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
>
>
