----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review96095 -----------------------------------------------------------
Can you rebase? clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java (line 130) <https://reviews.apache.org/r/36858/#comment151310> Returns a list of nodes with pending inflight requests that need to be timed out clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java (line 138) <https://reviews.apache.org/r/36858/#comment151311> Can do without this guard. clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java (line 141) <https://reviews.apache.org/r/36858/#comment151323> Is this right? i.e., `lastSent`? clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java (line 228) <https://reviews.apache.org/r/36858/#comment152086> This logic is a bit confusing. Is this block necessary here? i.e., vs being written once below? clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java (line 245) <https://reviews.apache.org/r/36858/#comment152089> I'm not sure we should remove the _replication timeout_ though. i.e., sure the replication timeout should not be used for request timeout going forward, but we still need a replication timeout in the producer request. clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java (line 428) <https://reviews.apache.org/r/36858/#comment152144> Minor point: given that there may be a custom serializer and custom partitioner, the elapsed check should probably be made after each step. clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java (line 100) <https://reviews.apache.org/r/36858/#comment152146> See comment above on the (continued) need for replication timeout. Also, can you fix whitespace on all your comments? i.e., `* @deprecated` instead of `*@deprecated`. clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java (line 196) <https://reviews.apache.org/r/36858/#comment152148> Let us make this doc string as exhaustive and clear as possible - i.e., drop the etc. and enumerate everything. You can also add something along the lines of - also see the request timeout config, with a doc link. clients/src/main/java/org/apache/kafka/clients/producer/internals/BufferPool.java (line 94) <https://reviews.apache.org/r/36858/#comment152158> Can you add the new param to the javadoc? clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java (line 155) <https://reviews.apache.org/r/36858/#comment152162> same here clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java (line 205) <https://reviews.apache.org/r/36858/#comment152168> Maybe drop the "due to...". This would be just one possibility right? E.g., you could have a low request timeout and high linger time... possibly other scenarios as well. clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java (line 214) <https://reviews.apache.org/r/36858/#comment152170> typo in comment clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java (line 221) <https://reviews.apache.org/r/36858/#comment152172> If you do a reverse iteration and a batch has _not_ expired, then we can break early right? - Joel Koshy On Aug. 12, 2015, 5:59 p.m., Mayuresh Gharat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36858/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2015, 5:59 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2120 > https://issues.apache.org/jira/browse/KAFKA-2120 > > > Repository: kafka > > > Description > ------- > > Solved compile error > > > Addressed Jason's comments for Kip-19 > > > Addressed Jun's comments > > > Addressed Jason's comments about the default values for requestTimeout > > > 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 > d35b421a515074d964c7fccb73d260b847ea5f00 > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > ed99e9bdf7c4ea7a6d4555d4488cf8ed0b80641b > > 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 > ce20111ac434eb8c74585e9c63757bb9d60a832f > clients/src/test/java/org/apache/kafka/clients/MockClient.java > 9133d85342b11ba2c9888d4d2804d181831e7a8e > 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 > clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java > 158f9829ff64a969008f699e40c51e918287859e > core/src/main/scala/kafka/tools/ProducerPerformance.scala > 0335cc64013ffe2cdf1c4879e86e11ec8c526712 > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala > ee94011894b46864614b97bbd2a98375a7d3f20b > core/src/test/scala/unit/kafka/utils/TestUtils.scala > eb169d8b33c27d598cc24e5a2e5f78b789fa38d3 > > Diff: https://reviews.apache.org/r/36858/diff/ > > > Testing > ------- > > > Thanks, > > Mayuresh Gharat > >
