sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Abhijeet Gaikwad" <abygaikwa...@gmail.com>
Subject Re: Review Request: SQOOP-529: Enforce usage of --driver and --connection-manager parameters
Date Wed, 01 Aug 2012 04:11:58 GMT

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



/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20641>

    "if" constructs are not defined consistently:
    At all places I see
    if (var <cond_op> constant)
    except at one place where it is:
    if (constant <cond_op> var).
    
    I personally suggest using second type of conditional check. 
    This comment is pretty minor though.



/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20625>

    If manager for a particular driver already exists, can we return that first here?
    E.g. - if driver name is - "com.mysql.jdbc.Driver", return MySqlManager that we already
have implemented.
    
    If no matching scenario, then we can return GenericJdbcManager.



/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20626>

    Please use "managerClassName" here which was extracted earlier in the code.



/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20629>

    Grammatical change:
    "Sqoop could not find specified Connection Manger class..."



/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20630>

    Grammatical change:
    please replace "are supporting" with "support".



/src/java/org/apache/sqoop/manager/DefaultManagerFactory.java
<https://reviews.apache.org/r/6250/#comment20638>

    I think this whole change is not required here. The control flows here only if manualDriver
and mangerClassName are NULL. Let me know if I am missing anything.


- Abhijeet Gaikwad


On July 31, 2012, 8:52 p.m., Jarek Cecho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6250/
> -----------------------------------------------------------
> 
> (Updated July 31, 2012, 8:52 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Description
> -------
> 
> I've refactored the code in the way I've designed in the JIRA.
> 
> 
> This addresses bug SQOOP-529.
>     https://issues.apache.org/jira/browse/SQOOP-529
> 
> 
> Diffs
> -----
> 
>   /src/java/org/apache/sqoop/ConnFactory.java 1367605 
>   /src/java/org/apache/sqoop/manager/DefaultManagerFactory.java 1367605 
> 
> Diff: https://reviews.apache.org/r/6250/diff/
> 
> 
> Testing
> -------
> 
> ant tests passes
> 
> 
> Thanks,
> 
> Jarek Cecho
> 
>


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