-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36858/#review94447
-----------------------------------------------------------
Thanks for the patch. A few comments below. Also, there seems to be style check
failure when running unit tests.
:clients:checkstyleMain[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java:22:8:
Unused import - java.util.Set.
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java:23:8:
Unused import - org.apache.kafka.common.utils.SystemTime.
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java:24:8:
Unused import - org.apache.kafka.common.utils.Time.
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java:164:100:
';' is preceded with whitespace.
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:223:
if child at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:223:
method call child at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:224:
method call child at indentation level 18 not at correct indentation, 20
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:225:
if child at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:226:
if at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:227:
if child at indentation level 16 not at correct indentation, 20
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:228:
if rcurly at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:229:
if child at indentation level 16 not at correct indentation, 20
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:229:
method call child at indentation level 16 not at correct indentation, 20
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:230:
method call child at indentation level 20 not at correct indentation, 24
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:231:
if child at indentation level 16 not at correct indentation, 20
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:232:
if rcurly at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:233:
else child at indentation level 16 not at correct indentation, 20
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:234:
else rcurly at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:236:
if child at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:236:
method call child at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:237:
method call child at indentation level 18 not at correct indentation, 20
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:238:
if child at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:240:
else child at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:248:
if child at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:250:
else child at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:429:
if child at indentation level 14 not at correct indentation, 16
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java:199:100:
';' is preceded with whitespace.
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/internals/BufferPool.java:128:
if child at indentation level 22 not at correct indentation, 24
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java:22:8:
Unused import - org.apache.kafka.common.errors.TimeoutException.
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java:169:
if at indentation level 18 not at correct indentation, 20
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java:170:
if child at indentation level 20 not at correct indentation, 24
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java:24:8:
Unused import - org.apache.kafka.common.utils.SystemTime.
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java:25:8:
Unused import - org.apache.kafka.common.utils.Time.
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java:194:
for child at indentation level 10 not at correct indentation, 12
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java:194:
method call child at indentation level 10 not at correct indentation, 12
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/common/network/Selector.java:182:9:
'{' should be on the previous line.
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/common/network/Selector.java:183:
if child at indentation level 10 not at correct indentation, 12
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/common/network/Selector.java:183:
method call child at indentation level 10 not at correct indentation, 12
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/common/network/Selector.java:184:
if child at indentation level 10 not at correct indentation, 12
[ant:checkstyle]
/Users/junrao/intellij/kafka/clients/src/main/java/org/apache/kafka/common/network/Selector.java:184:
method call child at indentation level 10 not at correct indentation, 12
FAILED
clients/src/main/java/org/apache/kafka/clients/NetworkClient.java (line 83)
<https://reviews.apache.org/r/36858/#comment149061>
To be consistent, requestTimeout can probably be requestTimeoutMs?
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
(line 248)
<https://reviews.apache.org/r/36858/#comment149059>
Should we add a similar warning that timeout_config will be deprecated?
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java
(line 50)
<https://reviews.apache.org/r/36858/#comment149055>
Should we add the @Deprecated annotation?
clients/src/main/java/org/apache/kafka/clients/producer/internals/BufferPool.java
(lines 127 - 129)
<https://reviews.apache.org/r/36858/#comment149062>
Coding style: no need to wrap single line statement with {}.
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java
(line 220)
<https://reviews.apache.org/r/36858/#comment149067>
Not sure if the test is needed. First, it seems that batch should never
will be null. Second, let's say the producer can't connect to any broker. The
producer can't refresh the metdata. So the leader will still be the old one and
may not be null. In this case, it seems that we should still expire the records.
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java
(line 225)
<https://reviews.apache.org/r/36858/#comment149070>
We can't remove from the iterator this way. Will get a
ConcurrentModificationException. Need to call iterator.remove.
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java
(lines 232 - 234)
<https://reviews.apache.org/r/36858/#comment149071>
Coding style on single line statement.
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java
(lines 128 - 133)
<https://reviews.apache.org/r/36858/#comment149066>
Merge into a single line?
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java
(line 137)
<https://reviews.apache.org/r/36858/#comment149073>
Hmm, not sure if the condition is completely right. According to the KIP
wiki, we want to expire a batch after it's been ready for more than request
timeout. The time when a batch is ready is lastAppendTime when the batch is
full. Otherwise, it's createTime + linger time.
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java
(lines 193 - 195)
<https://reviews.apache.org/r/36858/#comment149074>
Coding convention on single line statement.
clients/src/main/java/org/apache/kafka/common/network/Selector.java (line 182)
<https://reviews.apache.org/r/36858/#comment149075>
Coding convention: This should be in the previous line.
clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java
(line 314)
<https://reviews.apache.org/r/36858/#comment149094>
Should we try a request timeout larger than 0?
- Jun Rao
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
>
>
