----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18102/#review34437 -----------------------------------------------------------
Is there a case for failure and recovery. The scenario I am most interested in is: 1. bring up 2+ brokers 2. produce data to a topic with one partition 3. kill leader for that partition 4. produce data again (should fail since broker is down but we should initiate metadata refresh) 5. produce data again (should succeed on new leader) clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java <https://reviews.apache.org/r/18102/#comment64532> 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? clients/src/main/java/org/apache/kafka/clients/producer/internals/FutureRecordMetadata.java <https://reviews.apache.org/r/18102/#comment64633> 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... clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java <https://reviews.apache.org/r/18102/#comment64637> Does this mean reqacuiring the countdownlatch? core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment64638> The package doesn't match the directory. core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment64639> Is there a reason not to combine this with the other producer test? core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment64640> 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... core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment64642> This would probably make more sense as two tests, no? - Jay Kreps 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 > >
