> On July 27, 2015, 10:55 p.m., Jason Gustafson wrote: > > clients/src/main/java/org/apache/kafka/clients/ClientRequest.java, line 26 > > <https://reviews.apache.org/r/36858/diff/1/?file=1022752#file1022752line26> > > > > Should ClientResponse.requestLatencyMs be updated to use sendMs instead > > of createdMs? Or perhaps a new method like wireLatencyMs() can record > > responseMs - sendMs? Both seem like useful metrics.
Agreed with the wireLatency(). I would like to put it into a separate patch, since this is orthogonal to this KIP and we also need to think were we can use it. What do you think? > On July 27, 2015, 10:55 p.m., Jason Gustafson wrote: > > clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java, line > > 48 > > <https://reviews.apache.org/r/36858/diff/1/?file=1022754#file1022754line48> > > > > It seems like it might be more natural to set this in > > NetworkClient.send. That was my original thinking, but the problem is that if we set this in NetworkClient, we will have to set it in send() call and maybeUpdateMetadata() as well. Basically it looks a little loose since if there are new changes we might miss setting this. To avoid that we wanted to set this in a single place. > On July 27, 2015, 10:55 p.m., Jason Gustafson wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, line 86 > > <https://reviews.apache.org/r/36858/diff/1/?file=1022765#file1022765line86> > > > > Would it make sense to try to consolidate clientDisconnects and > > failedSends into the same collection? Unfortunately I can't think of a good > > name. Maybe "nextDisconnects"? I was thinking that it would be better, to separate out the request that are failed due to request timeout causing the client to close it explicitly and request that are failed because of other error between broker and client. Also clientDisconnects can be renamed to clientSideDisconnects to make it more explicit. What do you think? > On July 27, 2015, 10:55 p.m., Jason Gustafson wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 538 > > <https://reviews.apache.org/r/36858/diff/1/?file=1022758#file1022758line538> > > > > You don't necessarily need to fix this in this patch, but this setting > > could conflict with FETCH_MAX_WAIT_MS_CONFIG. Yup. It kind of does. I will create a separate jira for this. Also The requestTimeout in KafkaConsumer is long. It should be int. - Mayuresh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review93189 ----------------------------------------------------------- On July 27, 2015, 10:32 p.m., Mayuresh Gharat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36858/ > ----------------------------------------------------------- > > (Updated July 27, 2015, 10:32 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2120 > https://issues.apache.org/jira/browse/KAFKA-2120 > > > Repository: kafka > > > Description > ------- > > Kip-19 : Added RequestTimeout and MaxBlockTimeout as per the Kip > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/ClientRequest.java > ed4c0d98596cc294757f35df8c8cbc8e36ff42de > clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java > 2c421f42ed3fc5d61cf9c87a7eaa7bb23e26f63b > clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java > 15d00d4e484bb5d51a9ae6857ed6e024a2cc1820 > clients/src/main/java/org/apache/kafka/clients/KafkaClient.java > 7ab2503794ff3aab39df881bd9fbae6547827d3b > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java > 48fe7961e2215372d8033ece4af739ea06c6457b > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java > 70377ae2fa46deb381139d28590ce6d4115e1adc > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > bea3d737c51be77d5b5293cdd944d33b905422ba > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java > 9517d9d0cd480d5ba1d12f1fde7963e60528d2f8 > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 > clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java > aa264202f2724907924985a5ecbe74afc4c6c04b > > clients/src/main/java/org/apache/kafka/clients/producer/internals/BufferPool.java > 4cb1e50d6c4ed55241aeaef1d3af09def5274103 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java > a152bd7697dca55609a9ec4cfe0a82c10595fbc3 > > 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 > 0baf16e55046a2f49f6431e01d52c323c95eddf0 > clients/src/main/java/org/apache/kafka/common/network/Selector.java > aaf60c98c2c0f4513a8d65ee0db67953a529d598 > clients/src/test/java/org/apache/kafka/clients/MockClient.java > d9c97e966c0e2fb605b67285f4275abb89f8813e > clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java > 43238ceaad0322e39802b615bb805b895336a009 > > clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java > 2c693824fa53db1e38766b8c66a0ef42ef9d0f3a > > clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java > 5b2e4ffaeab7127648db608c179703b27b577414 > > clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java > 8b1805d3d2bcb9fe2bacb37d870c3236aa9532c4 > > Diff: https://reviews.apache.org/r/36858/diff/ > > > Testing > ------- > > > Thanks, > > Mayuresh Gharat > >
