sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boglarka Egyed <egyedb...@gmail.com>
Subject Re: Review Request 54206: Create a cmd line argument for sqoop.throwOnError and use it through SqoopOptions
Date Fri, 02 Dec 2016 10:59:37 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message