> 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. > > Mayuresh Gharat wrote: > 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?
Yeah, I think that's reasonable. > 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. > > Mayuresh Gharat wrote: > 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. Hmm.. It still sort of feels like it ought to be the responsibility of NetworkClient to set that and not InFlightRequests. An easy way to address the issue would be to create a NetworkClient.doSend method. For example: ``` void doSend(ClientRequest req) { req.setSendMs(time.milliseconds()); this.selector.send(req.request()); this.inFlightRequests.add(metadataRequest); } ``` Then NetworkClient.send and NetworkClient.maybeUpdateMetdata could invoke this method. Better or worse? > 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. > > Mayuresh Gharat wrote: > Yup. It kind of does. I will create a separate jira for this. Also The > requestTimeout in KafkaConsumer is long. It should be int. Sounds good. I'll drop this issue. > 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"? > > Mayuresh Gharat wrote: > 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? It would be nice to keep them separate if there was functional value in doing so, but it seems like they are used pretty much identically as a way to pass disconnects to the next round of poll. That's probably fine if it makes the code more understandable, and that might just be a matter of preference. What about "userDisconnects" instead of "clientSideDisconnects"? - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review93189 ----------------------------------------------------------- On July 29, 2015, 10:58 p.m., Mayuresh Gharat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36858/ > ----------------------------------------------------------- > > (Updated July 29, 2015, 10:58 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2120 > https://issues.apache.org/jira/browse/KAFKA-2120 > > > Repository: kafka > > > Description > ------- > > Patch for Kip-19 : Added RequestTimeOut and MaxBlockTimeOut > > Solved compile error > > > Addressed Jason's comments for Kip-19 > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/ClientRequest.java > dc8f0f115bcda893c95d17c0a57be8d14518d034 > 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 > 0e51d7bd461d253f4396a5b6ca7cd391658807fa > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java > 70377ae2fa46deb381139d28590ce6d4115e1adc > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > 923ff999d1b04718ddd9a9132668446525bf62f3 > > 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 > >
