> On Feb. 11, 2014, 12:49 a.m., Neha Narkhede wrote: > > 1. The JIRA attached to this rb is incorrect.
Yeah I made another, but since you commented on this one, I'll review here. > On Feb. 11, 2014, 12:49 a.m., Neha Narkhede wrote: > > perf/src/main/scala/kafka/perf/ProducerPerformance.scala, line 212 > > <https://reviews.apache.org/r/17921/diff/1/?file=482139#file482139line212> > > > > it is useful to differentiate entries from this tool. Can we set > > client.id to default to ProducerPerformance instead of an empty string? The problem is that when testing small messages (say 10 bytes), this was adding 19 extra bytes. Since we are network bound for many of the tests this is quite significant when we send one request per message. > On Feb. 11, 2014, 12:49 a.m., Neha Narkhede wrote: > > perf/src/main/scala/kafka/perf/ProducerPerformance.scala, line 208 > > <https://reviews.apache.org/r/17921/diff/1/?file=482139#file482139line208> > > > > NewShiny is nice. Is it worth specifying the version of the producer > > though? For example 08Producer and 09Producer for clarity? I don't think we should hard code the version number. It will be the 1.0 producer soon enough... :-) Basically this is meant as a short term fix for the next few releases while we maintain both clients so I think "new producer" should be ambiguous in that timeframe. > On Feb. 11, 2014, 12:49 a.m., Neha Narkhede wrote: > > perf/src/main/scala/kafka/perf/ProducerPerformance.scala, line 190 > > <https://reviews.apache.org/r/17921/diff/1/?file=482139#file482139line190> > > > > why remove ProducerPerformance from client.id? Same as the other case, since this is a performance test adding 19 bytes to each messages is pretty significant when you test 10 byte messages. Do we really need it? > On Feb. 11, 2014, 12:49 a.m., Neha Narkhede wrote: > > perf/src/main/scala/kafka/perf/ProducerPerformance.scala, line 123 > > <https://reviews.apache.org/r/17921/diff/1/?file=482139#file482139line123> > > > > Should we be explicit about the version (0.9) of the new producer in > > the doc? I don't think we should hard code the version number. It will be the 1.0 producer soon enough... :-) Basically this is meant as a short term fix for the next few releases while we maintain both clients so I think "new producer" should be ambiguous in that timeframe. > On Feb. 11, 2014, 12:49 a.m., Neha Narkhede wrote: > > core/src/test/scala/other/kafka/TestEndToEndLatency.scala, line 43 > > <https://reviews.apache.org/r/17921/diff/1/?file=482137#file482137line43> > > > > can we remove this comment? Yes, will do. > On Feb. 11, 2014, 12:49 a.m., Neha Narkhede wrote: > > clients/src/main/java/org/apache/kafka/clients/tools/ProducerPerformance.java, > > line 29 > > <https://reviews.apache.org/r/17921/diff/1/?file=482136#file482136line29> > > > > Do we still need this ProducerPerformance? > > > > Yes, the current producer test is too messed up to be trusted for performance testing. I don't believe anyone has actually run it through a profiler. It would be good to post mortum how that was allowed to happen but in the mean time I need this one. My hope was to get the new producer stable and then go back and clean up the existing performance test. That is harder than it sounds because it has become totally coupled to the integration testing in terms of output. > On Feb. 11, 2014, 12:49 a.m., Neha Narkhede wrote: > > bin/kafka-run-class.sh, line 73 > > <https://reviews.apache.org/r/17921/diff/1/?file=482134#file482134line73> > > > > this is not required anymore. After moving to gradle, we build > > kafka-clients*jar and kafka-run-class already includes that jar on the > > classpath Cool, I'll remove it. - Jay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17921/#review34135 ----------------------------------------------------------- On Feb. 10, 2014, 9:49 p.m., Jay Kreps wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17921/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2014, 9:49 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1255 > https://issues.apache.org/jira/browse/KAFKA-1255 > > > Repository: kafka > > > Description > ------- > > KAFKA-1236 Fix various breakages in the perf tests. Make the producer test > use either the old or the new producer. > > > Diffs > ----- > > bin/kafka-run-class.sh 75a3fc42a2e41977fa0d19a53cbc31e7538b8283 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > b274e5e4376b6bc7d1b54461d341e09d066bd5e2 > > clients/src/main/java/org/apache/kafka/clients/tools/ProducerPerformance.java > 108d61e6dba6d30654c38c829554234c29a2de36 > core/src/test/scala/other/kafka/TestEndToEndLatency.scala > c4aed10f50ca582fc51416625e46a53a1500ba28 > perf/src/main/scala/kafka/perf/ConsumerPerformance.scala > 55ee01b40afa43ecf0670471a96721f883fbedde > perf/src/main/scala/kafka/perf/ProducerPerformance.scala > ad2ac26411de80b498421e4557d4f22cff285f8a > project/Build.scala ddcfc4176e68933377590e095c07283083249f4a > > Diff: https://reviews.apache.org/r/17921/diff/ > > > Testing > ------- > > > Thanks, > > Jay Kreps > >
