----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58600/#review175233 -----------------------------------------------------------
Fix it, then Ship it! Hey Angela, Many thanks for providing this contribution. In general your changeset looks good, I've just found some codestyle issues, JDBC connection handling, and logging conventions. Could you please check them, and apply the changes / answer where I've raised questions? After we have the amendments I will be able to commit your changes! Many thanks, Attila src/java/org/apache/sqoop/manager/Db2Manager.java Lines 90-94 (patched) <https://reviews.apache.org/r/58600/#comment248686> Please fix indentation + formatting here! src/java/org/apache/sqoop/manager/Db2Manager.java Lines 181 (patched) <https://reviews.apache.org/r/58600/#comment248687> Is this commit call neded here? We've just only executed SELECT statement. What kind of changes we'd like to commit? src/java/org/apache/sqoop/manager/Db2Manager.java Lines 184 (patched) <https://reviews.apache.org/r/58600/#comment248688> I have the same concerns as around commit. Could you please give some more insights around why is this call needed? (FYI: I'm not a DB2 expert, so it's very possible I'm just not aware about the internals) src/java/org/apache/sqoop/manager/Db2Manager.java Lines 229 (patched) <https://reviews.apache.org/r/58600/#comment248674> Please remove the trailing whitesapce! src/java/org/apache/sqoop/manager/Db2Manager.java Lines 246-249 (patched) <https://reviews.apache.org/r/58600/#comment248689> Same concerns with commit+rollback as above src/java/org/apache/sqoop/manager/Db2Manager.java Lines 372-394 (patched) <https://reviews.apache.org/r/58600/#comment248685> Could you please check if this has been already solved in other connectors? Getting schema option sounds quite general, might have been implemented elsewhere in the codebase. Would make sense to abstract it out, and just call it from different places. Thanks for checking! src/java/org/apache/sqoop/manager/Db2Manager.java Lines 382 (patched) <https://reviews.apache.org/r/58600/#comment248675> Please remove trailing whitesapces! src/java/org/apache/sqoop/manager/Db2Manager.java Lines 393 (patched) <https://reviews.apache.org/r/58600/#comment248676> Please remove trailing whitesapces! src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java Lines 153 (patched) <https://reviews.apache.org/r/58600/#comment248677> Please remove trailing whitesapces! src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java Lines 166 (patched) <https://reviews.apache.org/r/58600/#comment248683> Please include debug level stack trace logging here! src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java Lines 233 (patched) <https://reviews.apache.org/r/58600/#comment248682> Please include debug level stack trace logging! src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java Lines 245 (patched) <https://reviews.apache.org/r/58600/#comment248681> Please include debug level stacktrace loggging here as well! src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java Lines 258 (patched) <https://reviews.apache.org/r/58600/#comment248680> How is this change related to the oraoop module? AFAIK DB2 execution path has to be independent from the oraoop execution path. What kind of problem did you face which led you to include this config settings? src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java Lines 264 (patched) <https://reviews.apache.org/r/58600/#comment248679> Please do remove printStackTrace calls. Logging the exception is the conventional way in the Sqoop codebase. If you'd like to add the stacktrace to the log as well (which is a best practice and very much advised), please add another line like: LOG.debug("Sqoop exception details", e); src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java Lines 270 (patched) <https://reviews.apache.org/r/58600/#comment248678> Please fix indendtation according to Sqoop guideline (here two leading spaces are missing) - Attila Szabo On April 21, 2017, 2:44 a.m., Ying Cao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58600/ > ----------------------------------------------------------- > > (Updated April 21, 2017, 2:44 a.m.) > > > Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, > Attila Szabo, and Szabolcs Vasas. > > > Bugs: SQOOP-1905 > https://issues.apache.org/jira/browse/SQOOP-1905 > > > Repository: sqoop-trunk > > > Description > ------- > > SQOOP-1905 : add --schema option for import-all-tables and list-tables > against db2 > > > Diffs > ----- > > src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef > src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/58600/diff/1/ > > > Testing > ------- > > Mannual UT is passed > > > Thanks, > > Ying Cao > >
