----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33552/#review82444 -----------------------------------------------------------
Some general comments: 1. This patch seems trying to fix multiple issues all at once, which makes it very hard to reason / review. Could you separate it into multiple phases with each phase resolving one issue at a time? 2. It would be best to not leaking network information such as metadata timeout into record accumlator, which is solely used for batching data and checking if some batched data is ready to be sent (regardless of whether their leaders are available, ready, or some metadata needs refresh, etc). 3. Some functions like partitionReady and batchReady are only triggered once in the code, and hence may actually be clearer to merge them into one place, or making the name to be more illustrative. For example batchReady returns the time for how long we should wait for this batch to be ready, while its name implicitly indicates returning a boolean. clients/src/main/java/org/apache/kafka/clients/NetworkClient.java <https://reviews.apache.org/r/33552/#comment133182> Is this change intentional? If yes what is the rational behind? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/33552/#comment133184> Do not use wildcards. - Guozhang Wang On May 2, 2015, 6:59 p.m., Jiangjie Qin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33552/ > ----------------------------------------------------------- > > (Updated May 2, 2015, 6:59 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2142 > https://issues.apache.org/jira/browse/KAFKA-2142 > > > Repository: kafka > > > Description > ------- > > Patch for KAFKA-2142 > > > Patch for KAFAK-2142 > > > Minor change in comments > > > Fix Null pointer > > > Add fix to KAFKA-1788 > > > Rebase on trunk > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/Metadata.java > 07f1cdb1fe920b0c7a5f2d101ddc40c689e1b247 > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java > b7ae595f2cc46e5dfe728bc3ce6082e9cd0b6d36 > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > d301be4709f7b112e1f3a39f3c04cfa65f00fa60 > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > 42b12928781463b56fc4a45d96bb4da2745b6d95 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java > 49a98838767615dd952da20825f6985698137710 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java > 06182db1c3a5da85648199b4c0c98b80ea7c6c0c > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > b2db91ca14bbd17fef5ce85839679144fff3f689 > clients/src/test/java/org/apache/kafka/clients/MetadataTest.java > 928087d29deb80655ca83726c1ebc45d76468c1f > clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java > 8b278892883e63899b53e15efb9d8c926131e858 > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/CoordinatorTest.java > b06c4a73e2b4e9472cd772c8bc32bf4a29f431bb > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java > 419541011d652becf0cda7a5e62ce813cddb1732 > > clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java > baa48e7c1b7ac5da8f3aca29f653c3fff88f8009 > > clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java > 8b1805d3d2bcb9fe2bacb37d870c3236aa9532c4 > clients/src/test/resources/log4j.properties > b1d5b7f2b4091040bdcfb0a60fd58111179f45a0 > core/src/test/resources/log4j.properties > 1b7d5d8f7d5fae7d272849715714781cad05d77b > > Diff: https://reviews.apache.org/r/33552/diff/ > > > Testing > ------- > > Unit Test passed. > > > Thanks, > > Jiangjie Qin > >
