Re: Review Request 36652: Patch for KAFKA-2351

2015-08-25 Thread Joel Koshy
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review96463 --- Ship it! Ship It! - Joel Koshy On Aug. 24, 2015, 10:50 p.m.,

Re: Review Request 36652: Patch for KAFKA-2351

2015-08-24 Thread Mayuresh Gharat
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- (Updated Aug. 24, 2015, 10:50 p.m.) Review request for kafka. Bugs:

Re: Review Request 36652: Patch for KAFKA-2351

2015-08-21 Thread Ismael Juma
On Aug. 21, 2015, 6:14 p.m., Joel Koshy wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 264 https://reviews.apache.org/r/36652/diff/4/?file=1039545#file1039545line264 Yes that's exactly what we need. Any reason why we shouldn't use `NonFatal` as is? i.e., if we

Re: Review Request 36652: Patch for KAFKA-2351

2015-08-21 Thread Ismael Juma
On Aug. 21, 2015, 5:57 p.m., Joel Koshy wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 264 https://reviews.apache.org/r/36652/diff/4/?file=1039545#file1039545line264 Thanks for the reference - we currently have this pattern all over the place. We can keep what

Re: Review Request 36652: Patch for KAFKA-2351

2015-08-21 Thread Joel Koshy
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review96069 --- core/src/main/scala/kafka/network/SocketServer.scala (line 264)

Re: Review Request 36652: Patch for KAFKA-2351

2015-08-21 Thread Joel Koshy
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review96061 --- core/src/main/scala/kafka/network/SocketServer.scala (line 264)

Re: Review Request 36652: Patch for KAFKA-2351

2015-08-19 Thread Mayuresh Gharat
On Aug. 19, 2015, 5:48 a.m., Joel Koshy wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 265 https://reviews.apache.org/r/36652/diff/4/?file=1039545#file1039545line265 I'm also unclear at this point on what the right thing to do here would be - i.e., log and

Re: Review Request 36652: Patch for KAFKA-2351

2015-08-19 Thread Mayuresh Gharat
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review95868 --- core/src/main/scala/kafka/network/SocketServer.scala (line 264)

Re: Review Request 36652: Patch for KAFKA-2351

2015-08-18 Thread Joel Koshy
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review95824 --- Patch needs a rebase.

Re: Review Request 36652: Patch for KAFKA-2351

2015-08-13 Thread Mayuresh Gharat
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- (Updated Aug. 13, 2015, 8:10 p.m.) Review request for kafka. Bugs:

Re: Review Request 36652: Patch for KAFKA-2351

2015-08-05 Thread Jun Rao
On July 24, 2015, 5:01 p.m., Mayuresh Gharat wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 264 https://reviews.apache.org/r/36652/diff/3/?file=1020607#file1020607line264 Hi Jun, Cleaning up in finally is actually nice. I will make the necessary change

Re: Review Request 36652: Patch for KAFKA-2351

2015-07-27 Thread Jiangjie Qin
On July 24, 2015, 4:13 p.m., Jun Rao wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 264 https://reviews.apache.org/r/36652/diff/3/?file=1020607#file1020607line264 Not sure if it's better to keep the thread alive on any throwable. For unexpected exceptions, it

Re: Review Request 36652: Patch for KAFKA-2351

2015-07-24 Thread Jun Rao
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review92920 --- Thanks for the patch. A few comments below.

Re: Review Request 36652: Patch for KAFKA-2351

2015-07-23 Thread Mayuresh Gharat
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- (Updated July 24, 2015, 4:36 a.m.) Review request for kafka. Bugs:

Re: Review Request 36652: Patch for KAFKA-2351

2015-07-21 Thread Jiangjie Qin
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review92488 --- Ship it! Latest patch looks good to me. - Jiangjie Qin On July

Re: Review Request 36652: Patch for KAFKA-2351

2015-07-21 Thread Mayuresh Gharat
On July 21, 2015, 8:18 p.m., Mayuresh Gharat wrote: T Yes. Got it, I thought that we should be catching all exceptions and exit. But doing the above will catch the exception and exit when its shutting down and thats the only thing that this ticket considers. - Mayuresh

Re: Review Request 36652: Patch for KAFKA-2351

2015-07-21 Thread Grant Henke
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review92470 --- core/src/main/scala/kafka/network/SocketServer.scala (line 266)

Re: Review Request 36652: Patch for KAFKA-2351

2015-07-21 Thread Mayuresh Gharat
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- (Updated July 21, 2015, 9:58 p.m.) Review request for kafka. Bugs:

Review Request 36652: Patch for KAFKA-2351

2015-07-21 Thread Mayuresh Gharat
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- Review request for kafka. Bugs: KAFKA-2351

Re: Review Request 36652: Patch for KAFKA-2351

2015-07-21 Thread Jiangjie Qin
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review92465 --- Thanks for the patch, some comments.

Re: Review Request 36652: Patch for KAFKA-2351

2015-07-21 Thread Grant Henke
On July 21, 2015, 8:26 p.m., Grant Henke wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 266 https://reviews.apache.org/r/36652/diff/1/?file=1018073#file1018073line266 What errors were seen that should be caught here? Can we catch a more specific exception and

Re: Review Request 36652: Patch for KAFKA-2351

2015-07-21 Thread Mayuresh Gharat
On July 21, 2015, 8:26 p.m., Grant Henke wrote: core/src/main/scala/kafka/network/SocketServer.scala, line 266 https://reviews.apache.org/r/36652/diff/1/?file=1018073#file1018073line266 What errors were seen that should be caught here? Can we catch a more specific exception and