----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54206/#review157727 -----------------------------------------------------------
Fix it, then Ship it! Hi Bogi, Your change looks okay. I've raised two questions in a fix it then ship it manner. Please answer them, or apply fixes if you agree with my arguments! Thanks! src/java/org/apache/sqoop/SqoopOptions.java (lines 1037 - 1043) <https://reviews.apache.org/r/54206/#comment228343> What about inline into something like this: this.throwOnError = (System.getProperty(SQOOP_RETHROW_PROPERTY) != null) Could you explain why do we prefer the broader format? src/java/org/apache/sqoop/tool/BaseSqoopTool.java (lines 286 - 290) <https://reviews.apache.org/r/54206/#comment228344> Why is it required to handle the RuntimeException differently? If I'm not mistaken in the old version we'd wrapped all type of exceptions, and no exclusion was applied to RuntimeExceptions? What is your intention here? - Attila Szabo On Dec. 2, 2016, 10:59 a.m., Boglarka Egyed wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54206/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2016, 10:59 a.m.) > > > Review request for Sqoop and Attila Szabo. > > > Bugs: SQOOP-3053 > https://issues.apache.org/jira/browse/SQOOP-3053 > > > Repository: sqoop-trunk > > > Description > ------- > > A new cmd line argument has been added for throwing a RuntimeException on > error which can be used through SqoopOptions from now. To ensure backward > compatibily the deafult value is set according to the sqoop.throwOnError > system property which was used for this feature before. > > > Diffs > ----- > > src/java/com/cloudera/sqoop/Sqoop.java > c3d9725b010f69be9b3664396d48974fb5f0d370 > src/java/org/apache/sqoop/Sqoop.java > 93736a6ba328648d5d6cbe6d53e917db52d304f9 > src/java/org/apache/sqoop/SqoopOptions.java > ef26f1628995bab5e8472339cc201d10e4093af2 > src/java/org/apache/sqoop/tool/BaseSqoopTool.java > 3ed0f779b047ff46277a1b2fe78b5666e5bff990 > src/java/org/apache/sqoop/tool/CodeGenTool.java > b3107a2b0eb5b794a1d21b8953c3854ed4cf67f4 > src/java/org/apache/sqoop/tool/CreateHiveTableTool.java > 427376d9fa226e33cb1a752e88f69603a1f7ffbd > src/java/org/apache/sqoop/tool/ExportTool.java > 89f859013880cba0639ed63201f1a1234767f39a > src/java/org/apache/sqoop/tool/ImportAllTablesTool.java > 0952c1125d99628591f956d160fb3a369e78c681 > src/java/org/apache/sqoop/tool/ImportTool.java > d3f8b935de95a541cb29240795502a4c663f2105 > src/java/org/apache/sqoop/tool/MergeTool.java > 09589d81c6529428031bb1bc56c3c5f9ae0906af > src/test/com/cloudera/sqoop/TestSqoopOptions.java > f9d1d54bd06531aae955d5dccc22e1d799cb1fb8 > src/test/org/apache/sqoop/tool/TestBaseSqoopTool.java PRE-CREATION > > Diff: https://reviews.apache.org/r/54206/diff/ > > > Testing > ------- > > New unit test cases have been added. > > > Thanks, > > Boglarka Egyed > >
