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 Tue, 21 Oct 2014 20:47:34 GMT

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



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

    Super nit: Can we also put the semicolon on it's own line? :)
    
    My goal here is to have ability to add new exception code by just adding a new line without
need to touch other lines which will make it easier for "git cherrypick" or "git blame".



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

    Seems as still the same invalid import? :)



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

    Thank you for cleaning this one!



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

    Do you think that it would make sense to change this call to Driver.getClass().getSimpleName()
so that we don't have to change it if at some point in the future we decides to change the
package or class name?



core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java
<https://reviews.apache.org/r/26941/#comment98457>

    I'm thinking if it would be claner to call here mDriver.getUniqueName() instead of going
to the Driver class directly?
    
    It's more a nitpick at this point, but this methods assumes that the mDriver will be derived
from Driver. Which right now will definitely be the case, but I'm wondering whether we want
to have such assumption here.



core/src/main/java/org/apache/sqoop/repository/Repository.java
<https://reviews.apache.org/r/26941/#comment98452>

    Seems as unused import?



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

    Can we keep the comment here so that it's obvious what we're doing here?



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

    Nit: Trailing whitespace



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

    Nit: Trailing whitespace



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

    Nit: Trailing whitespace



repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java
<https://reviews.apache.org/r/26941/#comment98462>

    Nit: Trailing whitespace


Jarcec

- Jarek Cecho


On Oct. 21, 2014, 7:19 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26941/
> -----------------------------------------------------------
> 
> (Updated Oct. 21, 2014, 7:19 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> see JIRA for details
> 
> there is whitespace, that will be addressed once the reviews for the functionality
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/model/MConnector.java 174d0b9 
>   common/src/main/java/org/apache/sqoop/model/MDriver.java 4241a31 
>   core/src/main/java/org/apache/sqoop/driver/Driver.java 46a16ac 
>   core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 476830d 
>   core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java 4c5229f 
>   core/src/main/java/org/apache/sqoop/repository/Repository.java 8f78052 
>   core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java ff9e0c3 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java
40dcc49 
>   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
aa58850 
>   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 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java
68a173b 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestDriverHandling.java
bbf721f 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java
a15bda9 
>   tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java 8cf9cf1 
> 
> Diff: https://reviews.apache.org/r/26941/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>


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