----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18740/#review36152 -----------------------------------------------------------
This is very good. I flagged a number of minor things, but basically this is great. clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java <https://reviews.apache.org/r/18740/#comment67066> Hey Guozhong, we aren't logging routine things as info. The plan we discussed was trace at the beginning and debug at the end. clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java <https://reviews.apache.org/r/18740/#comment67067> Ditto. clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java <https://reviews.apache.org/r/18740/#comment67068> Is there a reason this is seperate from the other member variables...? clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java <https://reviews.apache.org/r/18740/#comment67084> Could this be simplified to boolean notInBackoff = attempts = 0 || batch.lastAttempt + retryBackoffMs <= now and then add that boolean into the compound expression that wraps ready.add clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java <https://reviews.apache.org/r/18740/#comment67069> This should probably be called lastAttempt to mimic the other variable attempts. clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/18740/#comment67070> If you add TODOs please do make sure to do them :-) clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/18740/#comment67085> Let's make this "Got error for topic-partition {}, retrying ({} attempts left). Error: {}: clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/18740/#comment67086> We should not log this case as we are going to throw an exception. clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java <https://reviews.apache.org/r/18740/#comment67088> Alternately you can use an array for this where the ith entry in the array is the error with id i. This is what we do with the other cases. This way when you add a new api it is automatically added to the mapping which can be populated automatically by iterating over all elements in the enum. core/src/main/scala/kafka/log/LogManager.scala <https://reviews.apache.org/r/18740/#comment67071> Let's not add to log spam... core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala <https://reviews.apache.org/r/18740/#comment67089> Capitalization. - Jay Kreps On March 4, 2014, 7:05 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18740/ > ----------------------------------------------------------- > > (Updated March 4, 2014, 7:05 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1286 > https://issues.apache.org/jira/browse/KAFKA-1286 > > > Repository: kafka > > > Description > ------- > > 1. Fix the metadata-in-progree flag issue. > > 2. Add backoff config for retry. > > 3. Some logging level changes. > > 4. Fix a minor NPE bug in delete-topic-manager. > > 5. Rolling bounce test case. > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > bedd2a989a62b1ed53f006e7e2f8bd1bdc5dfa5b > clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java > d8e35e7d0e4cd27aad9a8d4bf14bc97458da9417 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java > 699027447145837495fb56b41ad9ee5e9cb60240 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java > c7fbf3c06858a6016878667b68ee29b22b604f7d > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > 794262394133d8e10e52971dccc0082d3aa75047 > clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java > 21a2592ea7c7f5d4831669196cf4e2d2b4e9bcf5 > core/src/main/scala/kafka/controller/KafkaController.scala > b58cdcd16ffb62ba5329b8b2776f2bd18440b3a0 > core/src/main/scala/kafka/log/LogManager.scala > 10062af1e02af5e4238f408ba5b9f98cc226244f > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala > 4b7c544594dba734c8875fce2a289f81d67ba291 > > Diff: https://reviews.apache.org/r/18740/diff/ > > > Testing > ------- > > integration tests > > > Thanks, > > Guozhang Wang > >
