-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54206/#review157573
-----------------------------------------------------------
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
src/test/com/cloudera/sqoop/TestSqoopOptions.java (line 482)
<https://reviews.apache.org/r/54206/#comment228200>
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.
- Szabolcs Vasas
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
>
>
