sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 26941: SQOOP-1557: SQ_CONFIGURABLE ( for entities who own configs)
Date Mon, 20 Oct 2014 18:26:30 GMT

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


Overall I like the changes. I do have couple of high level notes:

1) Please do remove all the trailing whitespaces. I've mark some of them. Most of the IDEs
do have option to automatically remove trailing whispaces on file save, so you might want
to enable that option :)

2) It seems that this patch also incorporates quite a lot of changes into the query names
and pretty much refactores the Query class. I don't mind doing those changes, but they should
be covered by a separate JIRA as they are unrelated to "rename SQ_CONFIGURABLE".


common/src/main/java/org/apache/sqoop/model/MConnector.java
<https://reviews.apache.org/r/26941/#comment98050>

    Seems as irrelevant import?



common/src/main/java/org/apache/sqoop/model/MConnector.java
<https://reviews.apache.org/r/26941/#comment98052>

    Why returning String? Would be better to return MConfigurableType directly?



common/src/main/java/org/apache/sqoop/model/MDriver.java
<https://reviews.apache.org/r/26941/#comment98053>

    Nit: Trailing white space



common/src/main/java/org/apache/sqoop/model/MDriver.java
<https://reviews.apache.org/r/26941/#comment98055>

    Why returning String, wouldn't it make more sense to return the enumb MConfigurableType
directly?



common/src/main/java/org/apache/sqoop/model/MDriver.java
<https://reviews.apache.org/r/26941/#comment98054>

    Nit: Trailing white space



core/src/main/java/org/apache/sqoop/driver/Driver.java
<https://reviews.apache.org/r/26941/#comment98056>

    Nit: trailing whitespace



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java
<https://reviews.apache.org/r/26941/#comment98057>

    Nit : Whitespace error



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java
<https://reviews.apache.org/r/26941/#comment98058>

    Super nit:
    
    Please put a comma at the end of the line and semicolon into it's own line. This way adding
a new error message means to add a new line instead of changing the existing line as well.
This will make tools such as git blame or git cherrypick much easier to use.



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/26941/#comment98059>

    Nit: Trailing whitespace



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/26941/#comment98060>

    Nit: Trailing whitespace



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/26941/#comment98061>

    Nit: Trailing whitespace



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
<https://reviews.apache.org/r/26941/#comment98062>

    Nit: Trailing whitespace



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java
<https://reviews.apache.org/r/26941/#comment98063>

    Nit: Trailing whitespace



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java
<https://reviews.apache.org/r/26941/#comment98064>

    Nit: Trailing whitespace



repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
<https://reviews.apache.org/r/26941/#comment98065>

    Nit: Trailing whitespace


Jarcec

- Jarek Cecho


On Oct. 20, 2014, 4:46 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26941/
> -----------------------------------------------------------
> 
> (Updated Oct. 20, 2014, 4:46 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see JIRA for details
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/model/MConfigurableType.java PRE-CREATION 
>   common/src/main/java/org/apache/sqoop/model/MConnector.java 2f42191 
>   common/src/main/java/org/apache/sqoop/model/MDriver.java 685439e 
>   core/src/main/java/org/apache/sqoop/driver/Driver.java f1b45bb 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java
3e4a4a9 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
c888910 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java
59773e1 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
44ec2e3 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java
366e4ee 
> 
> Diff: https://reviews.apache.org/r/26941/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


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