Re: Review Request 36652: Patch for KAFKA-2351
--- 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., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Addressed Joel's comments Diffs - core/src/main/scala/kafka/network/SocketServer.scala 649812d9f8014edbd9e99113a0f9eaf186360bcc Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description (updated) --- Addressed Joel's comments Diffs (updated) - core/src/main/scala/kafka/network/SocketServer.scala 649812d9f8014edbd9e99113a0f9eaf186360bcc Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
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 hit VM errors such as those in `NonFatal` then we should probably let it propagate anyway. Can you file a jira to do this across the board? There have been a few cases where we wanted to catch some fatal errors too (in `SocketServer`, if I remember correctly). In many (most?) cases `NonFatal` is probably OK though. Sure, I'll file a JIRA. - Ismael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review96069 --- On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Addressed Jun's comments Diffs - core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
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 you have, but this should probably become a utility say `CoreUtils.catchThrowable` (similar to `CoreUtils.swallow*`) I would suggest creating a `NonControl` extractor that is similar to `NonFatal` (https://github.com/scala/scala/blob/v2.11.7/src/library/scala/util/control/NonFatal.scala), but more limited. - Ismael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review96061 --- On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Addressed Jun's comments Diffs - core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- 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) https://reviews.apache.org/r/36652/#comment151273 Yes that's exactly what we need. Any reason why we shouldn't use `NonFatal` as is? i.e., if we hit VM errors such as those in `NonFatal` then we should probably let it propagate anyway. Can you file a jira to do this across the board? - Joel Koshy On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Addressed Jun's comments Diffs - core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- 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) https://reviews.apache.org/r/36652/#comment151266 Thanks for the reference - we currently have this pattern all over the place. We can keep what you have, but this should probably become a utility say `CoreUtils.catchThrowable` (similar to `CoreUtils.swallow*`) core/src/main/scala/kafka/network/SocketServer.scala (line 265) https://reviews.apache.org/r/36652/#comment151267 Yes, but we don't need to do that now. - Joel Koshy On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Addressed Jun's comments Diffs - core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
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 continue or make it fatal as Becket suggested. I'm leaning toward the latter but I agree we could revisit this. By fatal do you mean that we should shutdown the broker? - Mayuresh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review95824 --- On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Addressed Jun's comments Diffs - core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- 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) https://reviews.apache.org/r/36652/#comment151020 I referred this : https://www.sumologic.com/2014/05/05/why-you-should-never-catch-throwable-in-scala/ from the kafka 2353 patch. http://www.tzavellas.com/techblog/2010/09/20/catching-throwable-in-scala/ - Mayuresh Gharat On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Addressed Jun's comments Diffs - core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review95824 --- Patch needs a rebase. core/src/main/scala/kafka/network/SocketServer.scala (line 263) https://reviews.apache.org/r/36652/#comment150967 Per earlier comment - we don't need this right? core/src/main/scala/kafka/network/SocketServer.scala (line 264) https://reviews.apache.org/r/36652/#comment150968 Can you clarify why we need this? i.e., catch and rethrow without any logging? core/src/main/scala/kafka/network/SocketServer.scala (line 265) https://reviews.apache.org/r/36652/#comment150970 I'm also unclear at this point on what the right thing to do here would be - i.e., log and continue or make it fatal as Becket suggested. I'm leaning toward the latter but I agree we could revisit this. - Joel Koshy On Aug. 13, 2015, 8:10 p.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Addressed Jun's comments Diffs - core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description (updated) --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Addressed Jun's comments Diffs (updated) - core/src/main/scala/kafka/network/SocketServer.scala dbe784b63817fd94e1593136926db17fac6fa3d7 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
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 and upload a new patch. I was looking at the patch for : https://issues.apache.org/jira/browse/KAFKA-2353 as per the suggestions on the jira ticket. Was just curious if we can do the same there as well. We are catching all the Throwables and allowing the thread to continue processing. Is there something I am missing here. Mayuresh, Yes, saw the changes in KAFKA-2353. We can leave this the way that you did for now and revisit the catch throwable issue later. - Jun --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review92928 --- On July 24, 2015, 4:36 a.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Diffs - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
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 seems it's better to just propagate the exception, log it and then kill the thread. This is already done through Utils.newThread. If we want to clean things up (e.g. countdown) before propagating the exception, we can do that in a finally clause. Hi Jun, I think only letting the acceptor thread die might cause more issue. The broker in that case will not serve new connections. This essentially makes all the partitions on this broker become unavailable. If we let the accpetor thread exit, maybe we should shutdown the broker completely. - Jiangjie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review92920 --- On July 24, 2015, 4:36 a.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Diffs - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- 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. core/src/main/scala/kafka/network/SocketServer.scala (line 262) https://reviews.apache.org/r/36652/#comment147188 We don't interrupt the Acceptor thread. So this is unnecessary. core/src/main/scala/kafka/network/SocketServer.scala (line 264) https://reviews.apache.org/r/36652/#comment147192 Not sure if it's better to keep the thread alive on any throwable. For unexpected exceptions, it seems it's better to just propagate the exception, log it and then kill the thread. This is already done through Utils.newThread. If we want to clean things up (e.g. countdown) before propagating the exception, we can do that in a finally clause. - Jun Rao On July 24, 2015, 4:36 a.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Diffs - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description (updated) --- Added a try-catch to catch any exceptions thrown by the nioSelector Addressed comments on the Jira ticket Diffs (updated) - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- 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 21, 2015, 9:58 p.m., Mayuresh Gharat wrote: --- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Diffs - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
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 --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review92465 --- On July 21, 2015, 8:11 p.m., Mayuresh Gharat wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- (Updated July 21, 2015, 8:11 p.m.) Review request for kafka. Bugs: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Diffs - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- 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) https://reviews.apache.org/r/36652/#comment146669 What errors were seen that should be caught here? Can we catch a more specific exception and provide a better message? - Grant Henke On July 21, 2015, 8:11 p.m., Mayuresh Gharat wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- (Updated July 21, 2015, 8:11 p.m.) Review request for kafka. Bugs: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Diffs - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- 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: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Diffs (updated) - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Review Request 36652: Patch for KAFKA-2351
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- Review request for kafka. Bugs: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Diffs - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review92465 --- Thanks for the patch, some comments. core/src/main/scala/kafka/network/SocketServer.scala (lines 234 - 235) https://reviews.apache.org/r/36652/#comment146660 We probably want to keep the try-catch inside the while loop. core/src/main/scala/kafka/network/SocketServer.scala (line 235) https://reviews.apache.org/r/36652/#comment146661 Open source Kafka convention is to put the bracket on the same line. T - Jiangjie Qin On July 21, 2015, 8:11 p.m., Mayuresh Gharat wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- (Updated July 21, 2015, 8:11 p.m.) Review request for kafka. Bugs: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Diffs - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
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 provide a better message? Mayuresh Gharat wrote: The nioSelector can throw different exceptions : IOException, ClosedSelectorException, IllegalArgumentException. We can have different catch for each of them. But we thought that the log will telll us what exception was thrown when we pass it to error() I assumed you are adding this because you saw a specific error. I wasn't sure what error you saw and if more context could be given to the user. Perhaps the error you saw is fairly common during shutdown and should be ignored, and not logged at the error level. But all others should be handle as you are here. - Grant --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review92470 --- On July 21, 2015, 8:11 p.m., Mayuresh Gharat wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- (Updated July 21, 2015, 8:11 p.m.) Review request for kafka. Bugs: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Diffs - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
Re: Review Request 36652: Patch for KAFKA-2351
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 provide a better message? The nioSelector can throw different exceptions : IOException, ClosedSelectorException, IllegalArgumentException. We can have different catch for each of them. But we thought that the log will telll us what exception was thrown when we pass it to error() - Mayuresh --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/#review92470 --- On July 21, 2015, 8:11 p.m., Mayuresh Gharat wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36652/ --- (Updated July 21, 2015, 8:11 p.m.) Review request for kafka. Bugs: KAFKA-2351 https://issues.apache.org/jira/browse/KAFKA-2351 Repository: kafka Description --- Added a try-catch to catch any exceptions thrown by the nioSelector Diffs - core/src/main/scala/kafka/network/SocketServer.scala 91319fa010b140cca632e5fa8050509bd2295fc9 Diff: https://reviews.apache.org/r/36652/diff/ Testing --- Thanks, Mayuresh Gharat
