Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review101511 ---

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review101524 ---

Re: Review Request 36858: Patch for KAFKA-2120

> On Oct. 5, 2015, 6:51 p.m., Guozhang Wang wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 427 > > > > > > One minor thing: could you remove this comment? I had

Re: Review Request 36858: Patch for KAFKA-2120

> On Oct. 5, 2015, 8:34 p.m., Joel Koshy wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 427 > > > > > > LGTM - Guozhang, can you do a trivial commit to remove it?

Re: Review Request 36858: Patch for KAFKA-2120

> On Oct. 5, 2015, 6:51 p.m., Guozhang Wang wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 427 > > > > > > One minor thing: could you remove this comment? > >

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review101520 ---

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review100885 ---

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Sept. 28, 2015, 11:13 p.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review100880 --- core/src/main/scala/kafka/server/KafkaConfig.scala (line 67)

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 28, 2015, 11:04 p.m., Mayuresh Gharat wrote: > > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line > > 109 > > > > > > The request time outs in those classes are mainly used for

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 28, 2015, 11:04 p.m., Mayuresh Gharat wrote: > > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line > > 109 > > > > > > The request time outs in those classes are mainly used for

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Sept. 28, 2015, 11:16 p.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 19, 2015, 11:22 a.m., Ismael Juma wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 427 > > > > > > Can this TODO be removed? > > Mayuresh Gharat wrote: >

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 19, 2015, 11:22 a.m., Ismael Juma wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 427 > > > > > > Can this TODO be removed? > > Mayuresh Gharat wrote: >

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 19, 2015, 11:22 a.m., Ismael Juma wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 427 > > > > > > Can this TODO be removed? > > Mayuresh Gharat wrote: >

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review99669 ---

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review99661 ---

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review99659 --- clients/src/main/java/org/apache/kafka/clients/KafkaClient.java

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 19, 2015, 11:22 a.m., Ismael Juma wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 427 > > > > > > Can this TODO be removed? This wasn't a part of this

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 19, 2015, 11:22 a.m., Ismael Juma wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 427 > > > > > > Can this TODO be removed? > > Mayuresh Gharat wrote: >

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Sept. 19, 2015, 2:27 a.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review99235 --- Ship it! Thanks for the updated patch - these tests seem to fail

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review99098 --- Looks good overall - only have minor comments. Got this

Re: Review Request 36858: Patch for KAFKA-2120

> On Aug. 26, 2015, 8:20 p.m., Joel Koshy wrote: > > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java, > > line 230 > > > > > > This logic is a bit confusing. Is this block necessary here?

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Sept. 16, 2015, 1:57 a.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 11, 2015, 1:44 a.m., Jason Gustafson wrote: > > clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java, line > > 140 > > > > > > This line is puzzling me a little bit. Wouldn't we want to

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 11, 2015, 1:44 a.m., Jason Gustafson wrote: > > clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java, line > > 140 > > > > > > This line is puzzling me a little bit. Wouldn't we want to

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 10, 2015, 6:19 p.m., Ismael Juma wrote: > > clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java, line > > 137 > > > > > > Out of curiosity, why are we using `LinkedList` here? Generally

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 11, 2015, 1:44 a.m., Jason Gustafson wrote: > > clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java, line > > 140 > > > > > > This line is puzzling me a little bit. Wouldn't we want to

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Sept. 11, 2015, 9:54 p.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 9, 2015, 4:43 a.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, lines > > 185-190 > > > > > > Do we still need disconnect(id)? It seems that we can just

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review98438 ---

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 10, 2015, 6:19 p.m., Ismael Juma wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, line > > 488 > > > > > > Sorry if this has already been covered before, but why are we

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review98526 ---

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review98451 --- Ship it! Thanks for the patch. Just a minor comment below.

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 10, 2015, 6:19 p.m., Ismael Juma wrote: > > clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java, line > > 137 > > > > > > Out of curiosity, why are we using `LinkedList` here? Generally

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 11, 2015, 1:44 a.m., Jason Gustafson wrote: > > clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java, line > > 140 > > > > > > This line is puzzling me a little bit. Wouldn't we want to

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Sept. 11, 2015, 4:39 a.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

> On July 27, 2015, 10:55 p.m., Jason Gustafson wrote: > > clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java, line > > 48 > > > > > > It seems like it might be more natural to set this in > >

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Sept. 9, 2015, 11:49 p.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 9, 2015, 4:43 a.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, lines > > 185-190 > > > > > > Do we still need disconnect(id)? It seems that we can just

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 9, 2015, 4:43 a.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, lines > > 185-190 > > > > > > Do we still need disconnect(id)? It seems that we can just

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Sept. 9, 2015, 11:46 p.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review98327 ---

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Sept. 10, 2015, 1:56 a.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review98136 --- Thanks for the patch. A few more minor comments below.

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review97710 --- clients/src/main/java/org/apache/kafka/clients/NetworkClient.java

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 4, 2015, 4:07 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java, lines > > 362-367 > > > > > > But those closed connections are only added to the disconnected

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Sept. 5, 2015, 12:49 a.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review96965 ---

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 1, 2015, 1:02 a.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java, lines > > 362-367 > > > > > > We need to think through this logic a bit more. The patch here > >

Re: Review Request 36858: Patch for KAFKA-2120

> On Sept. 1, 2015, 1:02 a.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java, lines > > 362-367 > > > > > > We need to think through this logic a bit more. The patch here > >

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review97213 --- clients/src/main/java/org/apache/kafka/clients/NetworkClient.java

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review96095 --- Can you rebase?

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review96586 ---

Re: Review Request 36858: Patch for KAFKA-2120

On Aug. 11, 2015, 8:49 p.m., Jason Gustafson wrote: clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java, line 302 https://reviews.apache.org/r/36858/diff/4/?file=1037078#file1037078line302 Can we make this value greater than sessionTimeoutMs (which is

Re: Review Request 36858: Patch for KAFKA-2120

On Aug. 11, 2015, 8:49 p.m., Jason Gustafson wrote: clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java, line 302 https://reviews.apache.org/r/36858/diff/4/?file=1037078#file1037078line302 Can we make this value greater than sessionTimeoutMs (which is

Re: Review Request 36858: Patch for KAFKA-2120

--- 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:

Re: Review Request 36858: Patch for KAFKA-2120

On Aug. 11, 2015, 8:49 p.m., Jason Gustafson wrote: clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java, line 302 https://reviews.apache.org/r/36858/diff/4/?file=1037078#file1037078line302 Can we make this value greater than sessionTimeoutMs (which is

Re: Review Request 36858: Patch for KAFKA-2120

On Aug. 7, 2015, 12:36 a.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java, line 223 https://reviews.apache.org/r/36858/diff/3/?file=1024852#file1024852line223 Not sure if the test is needed. First, it seems that batch

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review94999 --- Ship it! LGTM (other than the minor issue below). As discussed on

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review94824 ---

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review94817 ---

Re: Review Request 36858: Patch for KAFKA-2120

On Aug. 7, 2015, 12:36 a.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java, line 223 https://reviews.apache.org/r/36858/diff/3/?file=1024852#file1024852line223 Not sure if the test is needed. First, it seems that batch

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated Aug. 11, 2015, 2:55 a.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

--- 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

Re: Review Request 36858: Patch for KAFKA-2120

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

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated July 29, 2015, 10:57 p.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

--- 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:

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated July 27, 2015, 9:09 p.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- (Updated July 27, 2015, 10:31 p.m.) Review request for kafka. Bugs:

Re: Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/#review93189 --- Looks pretty good overall. Found mostly trivial stuff.