----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review94817 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java (line 140) <https://reviews.apache.org/r/36858/#comment149453> Just out of curiosity: why this relience on use ``Iterable`` instead of using ``Collection`` or even ``List``. Didn't get why to use the highest interface is preferrable here. Again, just out of curiosity. clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java (line 141) <https://reviews.apache.org/r/36858/#comment149450> As jdk 1.7 is the current version then this line can be simplified to use diamond operators: List<String> nodeIds = new LinkedList<>(); clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java (line 142) <https://reviews.apache.org/r/36858/#comment149449> this if condition is unnecessary: if the map is empty the for-loop will NOT iterate a single time. clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java (line 154) <https://reviews.apache.org/r/36858/#comment149452> nit: I would wrap this on Collections.unmodifiableList() to make this List virtually immutable, in this particular case. clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java (line 209) <https://reviews.apache.org/r/36858/#comment149454> Use diamond operators: ``List<RecordBatch> expiredBatches = new ArrayList<>();`` clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java (line 216) <https://reviews.apache.org/r/36858/#comment149460> The lines can be simplified (i.e., more readable) a tidy bit. You can either write: ``` for (Iterator<RecordBatch> it = dq.iterator; it.hasNext; ) { RecordBatch batch = batchIterator.next(); Node leader = cluster.leaderFor(topicAndPartition); if (batch != null && leader == null) { // check if the batch is expired if (batch.expire(requestTimeout, now)) { expiredBatches.add(batch); count++; it.remove(); deallocate(batch); } } } ``` Or even simplier: ``` for (RecordBatch batch : dq) { Node leader = cluster.leaderFor(topicAndPartition); if (batch != null && leader == null) { // check if the batch is expired if (batch.expire(requestTimeout, now)) { expiredBatches.add(batch); count++; dq.remove(batch); deallocate(batch); } } } ``` clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java (line 236) <https://reviews.apache.org/r/36858/#comment149461> Again, we could wrap it in ``Collections.unmodifiableList``? (ps: I am asking, not stating). - Edward Ribeiro 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 > >
