Return-Path: X-Original-To: apmail-kafka-dev-archive@www.apache.org Delivered-To: apmail-kafka-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id BC38210769 for ; Tue, 4 Mar 2014 21:23:49 +0000 (UTC) Received: (qmail 19407 invoked by uid 500); 4 Mar 2014 21:23:49 -0000 Delivered-To: apmail-kafka-dev-archive@kafka.apache.org Received: (qmail 19325 invoked by uid 500); 4 Mar 2014 21:23:48 -0000 Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@kafka.apache.org Delivered-To: mailing list dev@kafka.apache.org Received: (qmail 19279 invoked by uid 99); 4 Mar 2014 21:23:48 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 04 Mar 2014 21:23:48 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id DDF141D4B8B; Tue, 4 Mar 2014 21:23:46 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============7435886064817072402==" MIME-Version: 1.0 Subject: Re: Review Request 18740: Fix KAFKA-1286 From: "Jay Kreps" To: "Jay Kreps" , "kafka" , "Guozhang Wang" Date: Tue, 04 Mar 2014 21:23:46 -0000 Message-ID: <20140304212346.6955.52925@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Jay Kreps" X-ReviewGroup: kafka X-ReviewRequest-URL: https://reviews.apache.org/r/18740/ X-Sender: "Jay Kreps" References: <20140304190531.10116.64664@reviews.apache.org> In-Reply-To: <20140304190531.10116.64664@reviews.apache.org> Reply-To: "Jay Kreps" X-ReviewRequest-Repository: kafka --===============7435886064817072402== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- 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 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 Ditto. clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java Is there a reason this is seperate from the other member variables...? clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java 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 This should probably be called lastAttempt to mimic the other variable attempts. clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java If you add TODOs please do make sure to do them :-) clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 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 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 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 Let's not add to log spam... core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 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 > > --===============7435886064817072402==--