----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26885/#review57513 -----------------------------------------------------------
Thanks for the patch. Looks good to me. Some minor comments below. clients/src/main/java/org/apache/kafka/clients/ClusterConnectionStates.java <https://reviews.apache.org/r/26885/#comment98242> It's probably better to put them in two lines. clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java <https://reviews.apache.org/r/26885/#comment98234> It seems that expired can just be timeLeftMs <= 0? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/26885/#comment98236> Are these comments redundant now given the new comments above? clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java <https://reviews.apache.org/r/26885/#comment98388> It seems that in this case, the nextReadyCheckDelayMs should be the remaining linger time for tp1, which is lingerMs/2. Should we just assert that? - Jun Rao On Oct. 21, 2014, 12:34 a.m., Ewen Cheslack-Postava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26885/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 12:34 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1642 > https://issues.apache.org/jira/browse/KAFKA-1642 > > > Repository: kafka > > > Description > ------- > > Fixes two issues with the computation of ready nodes and poll timeouts in > Sender/RecordAccumulator: > > 1. The timeout was computed incorrectly because it took into account all > nodes, > even if they had data to send such that their timeout would be 0. However, > nodes > were then filtered based on whether it was possible to send (i.e. their > connection was still good) which could result in nothing to send and a 0 > timeout, resulting in busy looping. Instead, the timeout needs to be computed > only using data that cannot be immediately sent, i.e. where the timeout will > be > greater than 0. This timeout is only used if, after filtering by whether > connections are ready for sending, there is no data to be sent. Other events > can > wake the thread up earlier, e.g. a client reconnects and becomes ready again. > > 2. One of the conditions indicating whether data is sendable is whether a > timeout has expired -- either the linger time or the retry backoff. This > condition wasn't accounting for both cases properly, always using the linger > time. This means the retry backoff was probably not being respected. > > KAFKA-1642 Compute correct poll timeout when all nodes have sendable data but > none can send data because they are in a connection backoff period. > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/ClusterConnectionStates.java > d304660f29246e9600efe3ddb28cfcc2b074bed3 > clients/src/main/java/org/apache/kafka/clients/KafkaClient.java > 29658d4a15f112dc0af5ce517eaab93e6f00134b > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java > eea270abb16f40c9f3b47c4ea96be412fb4fdc8b > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java > c5d470011d334318d5ee801021aadd0c000974a6 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > 8ebe7ed82c9384b71ce0cc3ddbef2c2325363ab9 > clients/src/test/java/org/apache/kafka/clients/MockClient.java > aae8d4a1e98279470587d397cc779a9baf6fee6c > > clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java > 0762b35abba0551f23047348c5893bb8c9acff14 > > Diff: https://reviews.apache.org/r/26885/diff/ > > > Testing > ------- > > > Thanks, > > Ewen Cheslack-Postava > >
