----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18102/#review35468 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java <https://reviews.apache.org/r/18102/#comment66026> it seems this constructor now is only used by the MockProducer. It is slightly awkward to have both these constructors. With the new one, can we make this one private and change all usages to point to the new constructor instead? core/src/main/scala/kafka/api/ProducerResponse.scala <https://reviews.apache.org/r/18102/#comment65999> Can this be DelayedProducerResponseStatus along the lines of ProducerResponseStatus. Also I'm not quite sure I understand the changes related to DelayedProduceResponseStatus. Let's chat about this. core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment66030> I'm trying to understand the scope of this test. It says failure handling and I'm not sure where you want to put the tests for testing the various ack modes in the presence of broker failures. One test I can think of is to have the producer send data using ack = -1 and validate the offsets returned and actual data to verify that there is no data loss. core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment66005> not-exist-topic -> non existing topics? cased -> caused core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment66006> cased -> caused core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment66007> that can be thrown core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment66012> cased -> caused core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment66014> Instead of too large, can we say sending a message >= bufferSize will cause the producer to throw a BufferExhaustedException? core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment66013> ditto core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment66021> is this TODO still required? core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment66023> can we remove this? core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18102/#comment66024> ditto here - Neha Narkhede On Feb. 20, 2014, 11:45 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18102/ > ----------------------------------------------------------- > > (Updated Feb. 20, 2014, 11:45 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1260 > https://issues.apache.org/jira/browse/KAFKA-1260 > > > Repository: kafka > > > Description > ------- > > Rebased on producer re-try. > > Incorporated Jay's fix for KAFKA-1266 > > Fixed some minor issues in producer: > > 1. Close accumulator on shutdown > > 2. Return offset for ack=-1 > > 3. Categorize error handling in send call > > Fixed the offset issue for ack=-1 on the server side, details in KAFKA-1255. > > Four test cases included in Part II: > > 1. testErrorInResponse. > > 2. testNoResponse. > > 3. testSendException. > > 4. testDeadBroker. Currently blocks.. > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > e4bc97279585818860487a39a93b6481742b91db > 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/Metadata.java > 62613a3e29a7e5ffb1cc56d267793fef72857fc6 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java > eb16f6d236e07b16654623606294a051531b5f58 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > e373265f19f6ec9d40b1813a1ab7e6b5b10e0acd > clients/src/main/java/org/apache/kafka/common/errors/FatalException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/errors/RecordTooLargeException.java > 737b7f07b16a0d6549e835c33c9dfad58008a590 > clients/src/main/java/org/apache/kafka/common/network/Selector.java > f1e474cd53011970c6bd3db6dfacd0f12ed9ce6b > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java > f88992a0cafd9639b4c5823aa80de1daf8b0eadd > core/src/main/scala/kafka/api/ProducerResponse.scala > 06261b9136399e48a8cb75f1c88a4cbfd1217de4 > core/src/main/scala/kafka/server/KafkaApis.scala > ae2df2014a08aaa95b5eaa430684cfdb79d4f55e > 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 > >
