Re: Review Request 36664: Patch for KAFKA-2353

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36664/#review92848 --- Ship it! Ship It! - Guozhang Wang On July 23, 2015, 12:51 a.m.,

Re: Review Request 36664: Patch for KAFKA-2353

> On July 22, 2015, 10:40 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 401 > > > > > > Is it intentional to ignore `java.lang.Error` too? > > Jiangjie Qin wrote: > I th

Re: Review Request 36664: Patch for KAFKA-2353

> On July 22, 2015, 10:40 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 401 > > > > > > Is it intentional to ignore `java.lang.Error` too? > > Jiangjie Qin wrote: > I th

Re: Review Request 36664: Patch for KAFKA-2353

> On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 465 > > > > > > Turns out that catching Throwable is a really bad idea: > > https://www.sumologic.co

Re: Review Request 36664: Patch for KAFKA-2353

> On July 22, 2015, 10:40 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 467 > > > > > > As far as I can see `ClosedChannelException`, `IllegalStateException` > > and `Illega

Re: Review Request 36664: Patch for KAFKA-2353

> On July 22, 2015, 10:40 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 467 > > > > > > As far as I can see `ClosedChannelException`, `IllegalStateException` > > and `Illega

Re: Review Request 36664: Patch for KAFKA-2353

> On July 22, 2015, 5:13 p.m., Guozhang Wang wrote: > > LGTM overall. Could you address Ismael's comments as well before check-in? Thanks, Guozhang. I updated patch to address Ismael's comments. - Jiangjie --- This is an automatically g

Re: Review Request 36664: Patch for KAFKA-2353

> On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 400 > > > > > > So in case of unexpected exception, we log an error and keep running? > > > >

Re: Review Request 36664: Patch for KAFKA-2353

> On July 22, 2015, 10:40 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 401 > > > > > > Is it intentional to ignore `java.lang.Error` too? I think java.lang.Error is a subcl

Re: Review Request 36664: Patch for KAFKA-2353

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36664/ --- (Updated July 23, 2015, 12:51 a.m.) Review request for kafka. Bugs: KAFKA-235

Re: Review Request 36664: Patch for KAFKA-2353

> On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 465 > > > > > > Turns out that catching Throwable is a really bad idea: > > https://www.sumologic.co

Re: Review Request 36664: Patch for KAFKA-2353

> On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 465 > > > > > > Turns out that catching Throwable is a really bad idea: > > https://www.sumologic.co

Re: Review Request 36664: Patch for KAFKA-2353

OK, I'm convinced :) I'll commit after Becket addresses other review points and adds some comments on how our error handling works (to make sure we don't screw it up later). Gwen On Wed, Jul 22, 2015 at 11:34 AM, Todd Palino wrote: > Since I've been dealing with the fallout of this particular p

Re: Review Request 36664: Patch for KAFKA-2353

Since I've been dealing with the fallout of this particular problem all week, I'll add a few thoughts... On Wed, Jul 22, 2015 at 10:51 AM, Gwen Shapira wrote: > > > > On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: > > > core/src/main/scala/kafka/network/SocketServer.scala, line 465 > > > < >

Re: Review Request 36664: Patch for KAFKA-2353

> On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 465 > > > > > > Turns out that catching Throwable is a really bad idea: > > https://www.sumologic.co

Re: Review Request 36664: Patch for KAFKA-2353

> On July 22, 2015, 10:40 a.m., Ismael Juma wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 467 > > > > > > As far as I can see `ClosedChannelException`, `IllegalStateException` > > and `Illega

Re: Review Request 36664: Patch for KAFKA-2353

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36664/#review92607 --- LGTM overall. Could you address Ismael's comments as well before che

Re: Review Request 36664: Patch for KAFKA-2353

> On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 400 > > > > > > So in case of unexpected exception, we log an error and keep running? > > > >

Re: Review Request 36664: Patch for KAFKA-2353

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36664/#review92574 --- core/src/main/scala/kafka/network/SocketServer.scala (line 401)

Re: Review Request 36664: Patch for KAFKA-2353

> On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: > > Thanks for looking into that. Exception handling was the most challenging > > part of rewriting SocketServer, so I'm glad to see more eyes on this > > implementation. > > > > I have a concern regarding the right way to handle an unexpecte

Re: Review Request 36664: Patch for KAFKA-2353

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36664/ --- (Updated July 22, 2015, 5:02 a.m.) Review request for kafka. Bugs: KAFKA-2353

Re: Review Request 36664: Patch for KAFKA-2353

> On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/network/SocketServer.scala, line 465 > > > > > > Turns out that catching Throwable is a really bad idea: > > https://www.sumologic.co

Re: Review Request 36664: Patch for KAFKA-2353

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36664/#review92496 --- Thanks for looking into that. Exception handling was the most challe

Review Request 36664: Patch for KAFKA-2353

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