> On Feb. 11, 2014, 7:18 p.m., Neha Narkhede wrote: > > core/src/test/resources/log4j.properties, line 15 > > <https://reviews.apache.org/r/17918/diff/2/?file=482287#file482287line15> > > > > this is probably included in the patch by accident right?
Yeah, will fix. > On Feb. 11, 2014, 7:18 p.m., Neha Narkhede wrote: > > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala, line 58 > > <https://reviews.apache.org/r/17918/diff/2/?file=482288#file482288line58> > > > > Naming it topic1 implies there will be topic2 as well. Can we stick to > > just topic? ack > On Feb. 11, 2014, 7:18 p.m., Neha Narkhede wrote: > > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala, line 86 > > <https://reviews.apache.org/r/17918/diff/2/?file=482288#file482288line86> > > > > we probably don't want to print anything from unit tests. These prints will only show up if the unit test runs with -i, otherwise they will not. I keep them here in case some exceptions are caught for running the callbacks. > On Feb. 11, 2014, 7:18 p.m., Neha Narkhede wrote: > > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala, line 180 > > <https://reviews.apache.org/r/17918/diff/2/?file=482288#file482288line180> > > > > What is the advantage of sending 10 times numRecords instead of just > > numRecords? This is just a magic number, I think numRecords should be sufficient, will fix > On Feb. 11, 2014, 7:18 p.m., Neha Narkhede wrote: > > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala, line 109 > > <https://reviews.apache.org/r/17918/diff/2/?file=482288#file482288line109> > > > > These 3 statements are repeated in many tests. Can we create a helper > > method to create topic successfully? Have thought about that, the reason I did not do so is that if I do this may goes to testUtils. But I think this is actually fine now, will fix. > On Feb. 11, 2014, 7:18 p.m., Neha Narkhede wrote: > > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala, line 211 > > <https://reviews.apache.org/r/17918/diff/2/?file=482288#file482288line211> > > > > Shouldn't the rest of the tests have acks set to -1 as well? ack = -1 is only required when I check the produced message content using a consumer. > On Feb. 11, 2014, 7:18 p.m., Neha Narkhede wrote: > > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala, line 219 > > <https://reviews.apache.org/r/17918/diff/2/?file=482288#file482288line219> > > > > why only wait until leader for partition 0 is elected? We should also > > probably wait until leader for partition 1 is elected so the test is not > > time dependent. For this test I only checks one partition, 0, which I think is sufficient. Not quite sure what you mean by "time dependent"? > On Feb. 11, 2014, 7:18 p.m., Neha Narkhede wrote: > > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala, line 242 > > <https://reviews.apache.org/r/17918/diff/2/?file=482288#file482288line242> > > > > The variable records for partition 0 is named record0. This is true for > > response as well. To be consistent, should this be fetchResponse0 for > > partition 0? ack > On Feb. 11, 2014, 7:18 p.m., Neha Narkhede wrote: > > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala, line 248 > > <https://reviews.apache.org/r/17918/diff/2/?file=482288#file482288line248> > > > > can we pull this logic in a method and do the same check for partition > > 1 as well? See above :) - Guozhang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17918/#review34158 ----------------------------------------------------------- On Feb. 11, 2014, 7:24 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17918/ > ----------------------------------------------------------- > > (Updated Feb. 11, 2014, 7:24 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1233 > https://issues.apache.org/jira/browse/KAFKA-1233 > > > Repository: kafka > > > Description > ------- > > Three test cases included in Part I: > > Incorporated Jay's comments. > > 1. Send API, with null key/value/partitionId/topic tested, check returned > offset. Currently passed. > > 2. All messages drained before close, also check that messages with the same > key go to the same partition. Currently passed. > > 3. The default round robin partitioner, and the specified partition id is > respected. Currently broken with ack = -1. Filed KAFKA-1255. > > 4. Send with auto-topic-creation. Currently passed. > > > Diffs > ----- > > build.gradle 858d297b9e8bf8a2bca54c4817f9ca2affd0d3f2 > core/src/test/resources/log4j.properties > d7d03ea1b13299d4ee41b9907720d014c81faf66 > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala > PRE-CREATION > project/Build.scala ddcfc4176e68933377590e095c07283083249f4a > > Diff: https://reviews.apache.org/r/17918/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
