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