Re: Review Request 36664: Patch for KAFKA-2353

2015-07-23 Thread Guozhang Wang
--- 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

2015-07-23 Thread Ismael Juma
On July 22, 2015, 10:40 a.m., Ismael Juma wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 401 https://reviews.apache.org/r/36664/diff/2/?file=1018375#file1018375line401 Is it intentional to ignore `java.lang.Error` too? Jiangjie Qin wrote: I think

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-23 Thread Jiangjie Qin
On July 22, 2015, 10:40 a.m., Ismael Juma wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 401 https://reviews.apache.org/r/36664/diff/2/?file=1018375#file1018375line401 Is it intentional to ignore `java.lang.Error` too? Jiangjie Qin wrote: I think

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-23 Thread Ismael Juma
On July 22, 2015, 10:40 a.m., Ismael Juma wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 467 https://reviews.apache.org/r/36664/diff/2/?file=1018375#file1018375line467 As far as I can see `ClosedChannelException`, `IllegalStateException` and

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-23 Thread Ismael Juma
On July 22, 2015, 10:40 a.m., Ismael Juma wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 467 https://reviews.apache.org/r/36664/diff/2/?file=1018375#file1018375line467 As far as I can see `ClosedChannelException`, `IllegalStateException` and

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-23 Thread Ismael Juma
On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 465 https://reviews.apache.org/r/36664/diff/1/?file=1018238#file1018238line465 Turns out that catching Throwable is a really bad idea:

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-22 Thread Guozhang Wang
On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 400 https://reviews.apache.org/r/36664/diff/1/?file=1018238#file1018238line400 So in case of unexpected exception, we log an error and keep running? Isn't it

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-22 Thread Gwen Shapira
On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 465 https://reviews.apache.org/r/36664/diff/1/?file=1018238#file1018238line465 Turns out that catching Throwable is a really bad idea:

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-22 Thread Guozhang Wang
--- 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

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-22 Thread Todd Palino
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 gshap...@cloudera.com wrote: On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: core/src/main/scala/kafka/network/SocketServer.scala,

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-22 Thread Jiangjie Qin
On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 465 https://reviews.apache.org/r/36664/diff/1/?file=1018238#file1018238line465 Turns out that catching Throwable is a really bad idea:

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-22 Thread Gwen Shapira
On July 22, 2015, 10:40 a.m., Ismael Juma wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 467 https://reviews.apache.org/r/36664/diff/2/?file=1018375#file1018375line467 As far as I can see `ClosedChannelException`, `IllegalStateException` and

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-22 Thread Ismael Juma
--- 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

2015-07-22 Thread Jiangjie Qin
On July 22, 2015, 10:40 a.m., Ismael Juma wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 401 https://reviews.apache.org/r/36664/diff/2/?file=1018375#file1018375line401 Is it intentional to ignore `java.lang.Error` too? I think java.lang.Error is a subclass of

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-22 Thread Jiangjie Qin
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

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-22 Thread Gwen Shapira
On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 465 https://reviews.apache.org/r/36664/diff/1/?file=1018238#file1018238line465 Turns out that catching Throwable is a really bad idea:

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-22 Thread Jiangjie Qin
--- 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:

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-21 Thread Jiangjie Qin
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 unexpected

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-21 Thread Jiangjie Qin
--- 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:

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-21 Thread Jiangjie Qin
On July 21, 2015, 11:15 p.m., Gwen Shapira wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 465 https://reviews.apache.org/r/36664/diff/1/?file=1018238#file1018238line465 Turns out that catching Throwable is a really bad idea:

Re: Review Request 36664: Patch for KAFKA-2353

2015-07-21 Thread Gwen Shapira
--- 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