sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Attila Szabo <mau...@apache.org>
Subject Re: Review Request 54206: Create a cmd line argument for sqoop.throwOnError and use it through SqoopOptions
Date Fri, 02 Dec 2016 11:09:24 GMT

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


Fix it, then Ship it!




Hi Bogi,

Your change looks okay. I've raised two questions in a fix it then ship it manner. Please
answer them, or apply fixes if you agree with my arguments!

Thanks!


src/java/org/apache/sqoop/SqoopOptions.java (lines 1037 - 1043)
<https://reviews.apache.org/r/54206/#comment228343>

    What about inline into something like this:
    this.throwOnError = (System.getProperty(SQOOP_RETHROW_PROPERTY) != null)
    
    Could you explain why do we prefer the broader format?



src/java/org/apache/sqoop/tool/BaseSqoopTool.java (lines 286 - 290)
<https://reviews.apache.org/r/54206/#comment228344>

    Why is it required to handle the RuntimeException differently?
    
    If I'm not mistaken in the old version we'd wrapped all type of exceptions, and no exclusion
was applied to RuntimeExceptions?
    
    What is your intention here?


- Attila Szabo


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