> On Aug. 15, 2015, 4:11 p.m., Jarek Cecho wrote: > > core/src/main/java/org/apache/sqoop/core/SqoopServer.java, lines 61-65 > > <https://reviews.apache.org/r/37474/diff/1/?file=1039990#file1039990line61> > > > > Since we moved to JDK7 now, what about having one catch clause for both > > cases? > > > > > > https://docs.oracle.com/javase/7/docs/technotes/guides/language/catch-multiple.html#multiple > > Colin Ma wrote: > From the FindBugs, the RuntimeException should be catched in theses code. > But the RuntimeException is a subclass of Exception, so the > catch-multiple doesn't work for this case. > I'll upate the patch to add log.error for RuntimeException. > > Jarek Cecho wrote: > I see, thanks for the expplanation Colin. I think that we've put the > Exception there to simply "catch all" case. Since findbugs is complaining > about it (and rightfully so), what about explicitly iterating what exceptions > we're catching? RuntimeException will definitely be on the list, not sure > what others. What do you think?
You're right, the patch is updated to catch the exact exception, thanks for your comments. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37474/#review95519 ----------------------------------------------------------- On Aug. 19, 2015, 5:29 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37474/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2015, 5:29 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Fix smaller-ish warnings in core module > > > Diffs > ----- > > core/src/main/java/org/apache/sqoop/audit/AuditLoggerManager.java 728127b > core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java a16fceb > > core/src/main/java/org/apache/sqoop/core/PropertiesConfigurationProvider.java > f1c4820 > core/src/main/java/org/apache/sqoop/core/SqoopServer.java e78e4d9 > core/src/main/java/org/apache/sqoop/driver/JobManager.java d4e0655 > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java c09b77b > > core/src/main/java/org/apache/sqoop/repository/JdbcTransactionIsolation.java > 2b1c8ce > core/src/main/java/org/apache/sqoop/repository/Repository.java 8cbff99 > > Diff: https://reviews.apache.org/r/37474/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
