> On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote: > > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala, > > line 67 > > <https://reviews.apache.org/r/18102/diff/2/?file=484693#file484693line67> > > > > You should be able to create this now. There is a TestUtil in the > > clients package. Either way cut and pasting each config three times isn't > > right...
The problem is that there are two TestUtils one in core and one in clients, so we cannot import both. > On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote: > > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala, > > line 36 > > <https://reviews.apache.org/r/18102/diff/2/?file=484693#file484693line36> > > > > Is there a reason not to combine this with the other producer test? I was trying to create more than one test suite for producer, each on different functionality. Another reason would just to not make the test file too big.. > On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote: > > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala, > > line 18 > > <https://reviews.apache.org/r/18102/diff/2/?file=484693#file484693line18> > > > > The package doesn't match the directory. Are we enforcing the match? I am not aware of this since the old test package does not enforce the match either. > On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote: > > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, > > line 221 > > <https://reviews.apache.org/r/18102/diff/2/?file=484686#file484686line221> > > > > We shouldn't use instanceof we should use separate catch clauses for > > each exception type, right? > > > > Also this isn't quite the logic we discussed. Shouldn't it be the case > > that we only catch ApiException? I thought the plan is to that send() can throw KafkaException, and future will contain ApiException. > On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote: > > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala, > > line 129 > > <https://reviews.apache.org/r/18102/diff/2/?file=484693#file484693line129> > > > > This would probably make more sense as two tests, no? I think this is a small enough test that we can wrap in one test case. > On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote: > > clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java, > > line 66 > > <https://reviews.apache.org/r/18102/diff/2/?file=484688#file484688line66> > > > > Is this thread safe? Is the only goal to avoid double-creating the > > RecordMetadata object? If so I wonder if this is actually better. Object > > creation is very fast and few people would end up using both the get() and > > the callback at the same time. > > > > The cleanup I was imagining was to explore the possibility of removing > > FutureRecordmetadata and replacing it with a generic SettableFuture and > > then setting the RecordMetadata instance on this future. Not sure if this > > would be better either... Yeah it is not thread safe. We can use an atomic reference to fix this. Another reason I just want one place to create the object is to avoid two places doing the same logic, since if we change one place in the future we have to remember to change the other (paranoid live at last)... Now after give it a second thought I am open to either the old way or the atomic reference. Not sure if the general SettableFuture works here? > On Feb. 14, 2014, 7:22 p.m., Jay Kreps wrote: > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java, > > line 82 > > <https://reviews.apache.org/r/18102/diff/2/?file=484689#file484689line82> > > > > Does this mean reqacuiring the countdownlatch? Yes, and if the countdown is already zero it should pass immediately. - Guozhang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18102/#review34437 ----------------------------------------------------------- On Feb. 13, 2014, 11:16 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18102/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2014, 11:16 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1260 > https://issues.apache.org/jira/browse/KAFKA-1260 > > > Repository: kafka > > > Description > ------- > > Fixed some minor issues in producer: > > 1. Close accumulator on shutdown > > 2. Return offset for ack=-1 > > 3. Categorize error handling in send call > > Four test cases included in Part II: > > 1. testErrorInResponse. > > 2. testNoResponse. Currently blocks.. > > 3. testSendException. > > 4. testDeadBroker. Currently failes > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > 3d180e885a25fa6b138f544ac320ec9e0d2a1e7f > clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java > 8c776980ef1f5167fb02dda36f6ad2385bd62bbd > > clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java > 22d4c79bc06fb77af30ab930855632c402c5a72e > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java > 7a440a3dd29c704dac9087fdac28c35d3e33e345 > > clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java > ce95ca04aa842078ad20ea3ae2c764b51ac76f7a > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java > f88992a0cafd9639b4c5823aa80de1daf8b0eadd > clients/src/test/java/org/apache/kafka/test/TestUtils.java > 36cfc0fda742eb501af2c2c0330e3f461cf1f40c > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala > PRE-CREATION > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala > 34baa8c6c7a15bb4aa93c286604f0eb7b19cd58e > core/src/test/scala/unit/kafka/utils/TestUtils.scala > 1c7a450651978e121376c226987b0d835f395a2a > > Diff: https://reviews.apache.org/r/18102/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
