Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated June 3, 2015, 10:59 p.m.) Review request for kafka. Bugs: 1928 and K

Re: Review Request 33065: Patch for KAFKA-1928

> On June 1, 2015, 11:52 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaConfig.scala, line 498 > > > > > > It's better to define it of type LIST, in the same way as the client. > > Then, we can use co

Re: Review Request 33065: Patch for KAFKA-1928

> On June 1, 2015, 11:52 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, line > > 600 > > > > > > We probably should include the metricsTag in the per-node sensor name >

Re: Review Request 33065: Patch for KAFKA-1928

> On June 1, 2015, 11:15 p.m., Jay Kreps wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, line > > 332 > > > > > > This is less of a "connection id" and more of a formatted connection,

Re: Review Request 33065: Patch for KAFKA-1928

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

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review86118 --- Ship it! Looks good to me. - Jay Kreps On May 31, 2015, 9:49 p.m

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review86117 --- clients/src/main/java/org/apache/kafka/common/network/Selector.java

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 31, 2015, 9:49 p.m.) Review request for kafka. Bugs: 1928 and KA

Re: Review Request 33065: Patch for KAFKA-1928

> On May 27, 2015, 6:44 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 109 > > > > > > Agreed with Joel's point. We should probably just create a single > > Metrics object in Kafk

Re: Review Request 33065: Patch for KAFKA-1928

> On May 27, 2015, 6:44 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 109 > > > > > > Agreed with Joel's point. We should probably just create a single > > Metrics object in Kafk

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review85412 --- core/src/main/scala/kafka/network/SocketServer.scala

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review85360 --- Ship it! Thanks for the big patch Gwen. core/src/main/scala/kafka

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review85180 --- Ship it! Thanks for the latest patch. +1 other than the following m

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 24, 2015, 4:53 p.m.) Review request for kafka. Bugs: 1928 and KA

Re: Review Request 33065: Patch for KAFKA-1928

> On May 21, 2015, 2:11 a.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/network/RequestChannel.scala, line 110 > > > > > > This would result in the following log entry: > > > > TRACE Completed > > r

Re: Review Request 33065: Patch for KAFKA-1928

> On May 21, 2015, 2:11 a.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/network/RequestChannel.scala, line 110 > > > > > > This would result in the following log entry: > > > > TRACE Completed > > r

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review84649 --- core/src/main/scala/kafka/network/RequestChannel.scala

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review84536 --- Thanks for the latest patch. Just two new minor comments and a coupl

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 20, 2015, 10:42 a.m.) Review request for kafka. Bugs: 1928 and K

Re: Review Request 33065: Patch for KAFKA-1928

> On May 19, 2015, 11:34 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 431 > > > > > > We probably just want to catch Exception. For unexpected error, we will > > just let it pro

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review84398 --- Thanks for the latest patch. Only a few more minor comments. Also,

Re: Review Request 33065: Patch for KAFKA-1928

> On May 18, 2015, 4:53 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Send.java, line 56 > > > > > > This needs to be long since NetworkSend always includes a size buffer, > > plus the

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 19, 2015, 8:26 a.m.) Review request for kafka. Bugs: 1928 and KA

Re: Review Request 33065: Patch for KAFKA-1928

> On May 18, 2015, 4:53 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, line > > 115 > > > > > > Do we need to turn this into a map? NetworkReceive already has the > > c

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review84158 --- Thanks for the latest patch. Looks good. Just a few minor comments b

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 18, 2015, 3:56 p.m.) Review request for kafka. Bugs: 1928 and KA

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 18, 2015, 2:58 p.m.) Review request for kafka. Bugs: 1928 and KA

Re: Review Request 33065: Patch for KAFKA-1928

> On May 15, 2015, 10:12 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 354 > > > > > > I guess we will make use of this in a followup patch? Actually, I think its no longer neede

Re: Review Request 33065: Patch for KAFKA-1928

> On May 15, 2015, 10:12 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/MultiSend.java, line > > 74 > > > > > > Actually, with MultiSend, we will be sending a 4-byte size plus the > >

Re: Review Request 33065: Patch for KAFKA-1928

> On May 15, 2015, 10:12 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java, > > lines 159-162 > > > > > > We don't need this since the idle conenctions only need to

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review83959 --- Thanks for the patch. There seem to be some compilation errors. Perh

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 15, 2015, 7:30 a.m.) Review request for kafka. Bugs: 1928 and KA

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 15, 2015, 12:07 a.m.) Review request for kafka. Changes ---

Re: Review Request 33065: Patch for KAFKA-1928

> On May 6, 2015, 10:50 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, lines > > 311-315 > > > > > > In this case, we should probably just propagate the exception and >

Re: Review Request 33065: Patch for KAFKA-1928

> On May 6, 2015, 10:50 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, lines > > 311-315 > > > > > > In this case, we should probably just propagate the exception and >

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 14, 2015, 9:30 p.m.) Review request for kafka. Changes ---

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review83841 --- core/src/main/scala/kafka/network/SocketServer.scala

Re: Review Request 33065: Patch for KAFKA-1928

> On May 6, 2015, 10:50 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, lines > > 311-315 > > > > > > In this case, we should probably just propagate the exception and >

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 14, 2015, 9:51 p.m.) Review request for kafka. Changes ---

Re: Review Request 33065: Patch for KAFKA-1928

> On May 6, 2015, 10:50 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 455 > > > > > > This is just registering for reads, not for writes, right? Maybe I misunderstood, but it look

Re: Review Request 33065: Patch for KAFKA-1928

> On May 6, 2015, 10:50 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, lines > > 198-199 > > > > > > Could we just iterate the keySet directly instead of making a copy >

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 12, 2015, 9:58 a.m.) Review request for kafka. Bugs: 1928 and KA

Re: Review Request 33065: Patch for KAFKA-1928

> On May 6, 2015, 10:50 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/ByteBufferSend.java, > > lines 55-64 > > > > > > Since we always use a 4 byte size, perhaps we should make both >

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 12, 2015, 9 a.m.) Review request for kafka. Summary (updated) --

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review82347 --- clients/src/main/java/org/apache/kafka/clients/producer/internals/S

Re: Review Request 33065: Patch for KAFKA-1928

> On April 18, 2015, 12:49 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/api/FetchResponse.scala, lines 66-72 > > > > > > I am wondering if we need both completed() and remaining() in Send. It > > seems that one o

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review82625 --- Thanks for the patch. Looks great overall! Some comments below. Als

Re: Review Request 33065: Patch for KAFKA-1928

> On May 1, 2015, 9:25 p.m., Jay Kreps wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 382 > > > > > > Beautiful! So much deleted code! Haha :) Yeah, I love how minimal SocketServer became. - G

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 1, 2015, 10:45 p.m.) Review request for kafka. Bugs: KAFKA-1928

Re: Review Request 33065: Patch for KAFKA-1928

> On April 18, 2015, 12:49 a.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/ByteBufferSend.java, > > line 55 > > > > > > Do you know why the return is changed from int to long? writeTo()

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review82281 --- I did a high level pass. On the whole I think this is awesome! I agr

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated May 1, 2015, 12:48 a.m.) Review request for kafka. Bugs: KAFKA-1928

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/ --- (Updated April 28, 2015, 7:09 a.m.) Review request for kafka. Bugs: KAFKA-192

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review80761 --- clients/src/main/java/org/apache/kafka/common/network/NetworkReceiv

Re: Review Request 33065: Patch for KAFKA-1928

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33065/#review80557 --- Took a quick look. Overall, looks good. A few comments below. clie