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

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? (O

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 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsu

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 ask

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 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsu

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 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsu

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 S

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 S

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

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

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 --- core/src/main/scala/kafka/controller/ControllerChannelManager.scal

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

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

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 --- clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsum

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 --- core/src/main/scala/kafka/controller/ControllerChannelManager.scala

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

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

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 co

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

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 compilation

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 us

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 us

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

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 us

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

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

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 us

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 --- clients/src/main/java/org/apache/kafka/clients/InFlightRequests.jav

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 logg

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

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 logg

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 --- clients/src/main/java/org/apache/kafka/clients/InFlightRequests.jav

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

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 replace

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

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 --- clients/src/main/java/org/apache/kafka/clients/producer/internals/R

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 replace

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

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 replace

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

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. clients/src

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

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 list

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

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

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. 3, 2015, 10:12 p.m.) Review request for kafka. Bugs: KAFKA-212

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/#review96586 --- clients/src/main/java/org/apache/kafka/clients/producer/internals/R

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? clients/src/main/java/org/apache/kafka/clients/InF

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 > > > > > > Can we make this value greater than sessionTimeoutMs (w

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

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 > > > > > > Can we make this value greater than sessionTimeoutMs (w

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 > > > > > > Can we make this value greater than sessionTimeoutMs (w

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 t

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 > > > > > > Not sure if the test is needed. First, it seems th

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

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 > > > > > > Not sure if the test is needed. First, it seems th

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 --- clients/src/main/java/org/apache/kafka/clients/producer/internals/R

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 --- clients/src/main/java/org/apache/kafka/clients/InFlightRequests.jav

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 > > > > > > Should ClientResponse.requestLatencyMs be updated to use sendMs instead

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

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

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 > > > > > > Should ClientResponse.requestLatencyMs be updated to use sendMs instead

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. clients/src

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:32 p.m.) Review request for kafka. Bugs: KAFKA-212

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

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

Review Request 36858: Patch for KAFKA-2120

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36858/ --- Review request for kafka. Bugs: KAFKA-2120 https://issues.apache.org/jira/b