> On Dec. 1, 2016, 11:20 a.m., Szabolcs Vasas wrote: > > Hi Bogi! > > > > Thank you for your patch! I have left one minor comment for your new test > > cases and I think you could introduce a new method instead of this block: > > > > if (options.isThrowOnError()) { > > throw new RuntimeException(ie); > > } else { > > return 1; > > } > > > > This seems to be duplicated several times in the code, I know it is not > > entirely yours but it may be a good opportunity to refactor it. > > > > Thanks, > > Szabolcs > > Attila Szabo wrote: > Absolutely agree with Szabolcs! Would be a nice extraction.
Thanks Szabi, this makes sense, I have made some refactor and uploaded a new patch. Please review it too, thanks! > On Dec. 1, 2016, 11:20 a.m., Szabolcs Vasas wrote: > > src/test/com/cloudera/sqoop/TestSqoopOptions.java, line 482 > > <https://reviews.apache.org/r/54206/diff/1/?file=1573023#file1573023line482> > > > > I think you can use assertTrue(opts.isThrowOnError()) here as it would > > be a bit simpler and it would be consistent with > > testThrowOnErrorWithNotSetSystemProperty and > > testThrowOnErrorWithSetSystemProperty test cases. > > Attila Szabo wrote: > +1 for the comment. Thanks Szabolcs! Thanks for the review, Attila! - Boglarka ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54206/#review157573 ----------------------------------------------------------- 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 > >