> 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

Absolutely agree with Szabolcs! Would be a nice extraction.


> 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.

+1 for the comment. Thanks Szabolcs!


- Attila


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54206/#review157573
-----------------------------------------------------------


On Nov. 30, 2016, 2:26 p.m., Boglarka Egyed wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54206/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2016, 2:26 p.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 
> 
> Diff: https://reviews.apache.org/r/54206/diff/
> 
> 
> Testing
> -------
> 
> New unit test cases have been added.
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Reply via email to