> On Oct. 23, 2014, 9:43 p.m., Jun Rao wrote: > > clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java, > > lines 197-199 > > <https://reviews.apache.org/r/26885/diff/2/?file=726776#file726776line197> > > > > 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?
tp1 and tp2 have the same leader, node1. The test adds enough data to make tp2 sendable, so in the ideal case only tp3 would be used to determine timeout, which should require lingerMs more time. However, the test checks for <= lingerMs because single scan through the topic partitions means that we can incorporate the lingerMs/2 timeout from tp1 even though we determine later that we really want to ignore it (and I actually saw this happen when I initially wrote the test to check for the exact value). I think the tradeoff of sometimes waking up a bit earlier than needed is probably worthwhile since it keeps the implementation simpler and cheaper. - Ewen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26885/#review57513 ----------------------------------------------------------- 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 > >
