----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33552/#review84191 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java <https://reviews.apache.org/r/33552/#comment135322> This could be private function. clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java <https://reviews.apache.org/r/33552/#comment135323> This could be private. clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/33552/#comment135328> This piece of comments are not very clear. Some suggestions: 1. Put the comments from "// When should ..." to "Otherwise ..." together, and add space before "If we have..." 2. The sentence "Note that .." is quite confusing as "sendable data that aren't ready to send". Better change it to "Otherwise the timeout is determined by the smaller of node connection delay (e.g. reconnecting backoff) and the ready-nodes' data yet-sendable delay (e.g. lingering, resending backoff)". clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java <https://reviews.apache.org/r/33552/#comment135331> To avoid wildcard in your Intellij' IDE, you can set the wildcard imports threshold to be very high, like 99 (in settings -> code style -> java -> imports). clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java <https://reviews.apache.org/r/33552/#comment135332> Ditto above. clients/src/test/resources/log4j.properties <https://reviews.apache.org/r/33552/#comment135316> Intentional? core/src/test/resources/log4j.properties <https://reviews.apache.org/r/33552/#comment135317> Intentional? Regarding the first issue of "linger.ms-not-honored": it is actually intentionally designed such that if one partition of a node is ready, we just get all its partitions while sending the request to the node for better network throughput. But I understand that ther is a trade-off of reducing the compression effect on other partitions, and would like Jay/Jun to comment on this manner. - Guozhang Wang On May 5, 2015, 1:15 a.m., Jiangjie Qin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33552/ > ----------------------------------------------------------- > > (Updated May 5, 2015, 1:15 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2142 > https://issues.apache.org/jira/browse/KAFKA-2142 > > > Repository: kafka > > > Description > ------- > > Somehow lost the comments when used kafka-patch-review tool... Adding them > back. > > This patch tries to solve the following issues: > > 1. Linger.ms is not honored - If partition A of node1 is not ready but > partiton B is ready, data of partiton A will also be sent out. > 2. Retry backoff is not honord - A retry batch could wait for another > linger.ms instead of retry.backoff.ms > > Changed the following behavior: > 1. Drain the data then check when next time data will be ready. > 2. Only refresh metadata if we have a partition with data to send but we > don't know its leader. > > Added the following behavior: > 1. Expire a batch if its leader is unknown for metadata timeout. > > > Addressed Guozhang's comments. > > > 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 > >
