kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jay Kreps" <boredandr...@gmail.com>
Subject Re: Review Request 18740: Fix KAFKA-1286
Date Tue, 04 Mar 2014 21:23:46 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message