> 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: > > https://www.sumologic.com/2014/05/05/why-you-should-never-catch-throwable-in-scala/ > > Jiangjie Qin wrote: > Ah... Didn't know that before. I explicitly listed the exceptions. > > Guozhang Wang wrote: > Searching ": Throwable" gives me 180+ cases in code base :P Though many > of them are from unit tests (which, arguably maybe OK) there are still a lot > in the core package. I agree that we should avoid catching Throwable whenever > possible, which will also help enforcing the developers to think about > possible checked exceptions in the calling trace. > > Gwen Shapira wrote: > I know :( > I'm not sure if going over and converting everything is worth the effort. > Although it can be a nice newbie jira. > > Jiangjie Qin wrote: > Maybe we can simply change that to catch Throwables except > ControlThrowables? That might be a simple search and replace. > > Gwen Shapira wrote: > possible. definitely not in this JIRA though :)
It's worth noting that the `ControlThrowable` thing is only an issue if `return` is used inside a closure or for comprehension and `return` is something to be avoided in Scala. Still, it would be nice to fix the issue. If we do, we should not add an additional clause everywhere. Instead, we should add an extractor like `NonFatal` (maybe called `NonControl`) to avoid unnecessary boilerplate. - Ismael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36664/#review92496 ----------------------------------------------------------- On July 23, 2015, 12:51 a.m., Jiangjie Qin wrote: > > ----------------------------------------------------------- > 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-2353 > https://issues.apache.org/jira/browse/KAFKA-2353 > > > Repository: kafka > > > Description > ------- > > Addressed Gwen's comments > > > Addressed Gwen's comments. > > > Diffs > ----- > > core/src/main/scala/kafka/network/SocketServer.scala > 91319fa010b140cca632e5fa8050509bd2295fc9 > > Diff: https://reviews.apache.org/r/36664/diff/ > > > Testing > ------- > > > Thanks, > > Jiangjie Qin > >
