sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Qian Xu" <sx.a...@googlemail.com>
Subject Re: Review Request 27085: SQOOP-1589: Sqoop2: Create common constants, error codes, and queries
Date Fri, 24 Oct 2014 05:15:19 GMT

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



core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
<https://reviews.apache.org/r/27085/#comment99194>

    `LOG.isDebugEnabled()` is this really necessary? I'd suggest use parameterized logging
rather than add if a if-clausel. http://www.slf4j.org/faq.html#logging_performance



core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
<https://reviews.apache.org/r/27085/#comment99195>

    For better code understanding, maybe better group `updateConnectorStatement` `deleteInput`
and `deleteConfig` in three blocks?
    
    deleteInput = conn.prepareStatement(STMT_DELETE_INPUTS_FOR_CONFIGURABLE);
    deleteInput.setLong(1, mConnector.getPersistenceId());
    deleteInput.executeUpdate();
    
    deleteConfig = conn.prepareStatement(STMT_DELETE_CONFIGS_FOR_CONFIGURABLE);
    deleteConfig.setLong(1, mConnector.getPersistenceId());
    deleteConfig.executeUpdate();



core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
<https://reviews.apache.org/r/27085/#comment99196>

    Can we use `closeStatements()` as other classes do?



core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
<https://reviews.apache.org/r/27085/#comment99198>

    Nice to have:
    
    `return submission != null && submission.getStatus().isRunning();`



core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
<https://reviews.apache.org/r/27085/#comment99199>

    How about create local `ResultSet` (and close it locally) instead creating one `ResultSet`
out of the for-block?
    
    please search for similar occurences



core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
<https://reviews.apache.org/r/27085/#comment99202>

    no close resultset? (please search for similar occurences)



core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
<https://reviews.apache.org/r/27085/#comment99206>

    General question, if check null is required, then please add the check to all code occurences,
otherwise please remove unused null check.



core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
<https://reviews.apache.org/r/27085/#comment99208>

    General question: please make methods static if it is possible.


- Qian Xu


On Oct. 24, 2014, 1:13 a.m., Abraham Elmahrek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27085/
> -----------------------------------------------------------
> 
> (Updated Oct. 24, 2014, 1:13 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-158
>     https://issues.apache.org/jira/browse/SQOOP-158
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> commit 370f476b1834c390993b31007b0fc7e1a184b891
> Author: Abraham Elmahrek <abraham@elmahrek.com>
> Date:   Wed Oct 22 18:30:28 2014 -0700
> 
>     SQOOP-1589: Sqoop2: Create common constants, error codes, and queries
> 
> :100644 100644 7d78826... e41cd6c... M  core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
> :000000 100644 0000000... a7ebd50... A  core/src/main/java/org/apache/sqoop/repository/JdbcRepositorySchemaConstants.java
> :000000 100644 0000000... eb05567... A  core/src/main/java/org/apache/sqoop/repository/JdbcRepositorySchemaQuery.java
> :100644 100644 f684e85... 097a185... M  core/src/main/java/org/apache/sqoop/repository/RepositoryError.java
> :100644 100644 aad219e... 769544b... M  repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java
> :100644 100644 633e9df... 6764b14... M  repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
> :100644 100644 ce1830e... ff8962c... M  repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
> :100644 100644 85140d5... aab1e95... M  repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java
> :100644 100644 37343d3... 9bcf3c8... M  repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java 7d78826 
>   core/src/main/java/org/apache/sqoop/repository/JdbcRepositorySchemaConstants.java PRE-CREATION

>   core/src/main/java/org/apache/sqoop/repository/JdbcRepositorySchemaQuery.java PRE-CREATION

>   core/src/main/java/org/apache/sqoop/repository/RepositoryError.java f684e85 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java
aad219e 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
633e9df 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
ce1830e 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java
85140d5 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java
37343d3 
> 
> Diff: https://reviews.apache.org/r/27085/diff/
> 
> 
> Testing
> -------
> 
> mvn clean verify + start sqoop server without issues.
> 
> 
> Thanks,
> 
> Abraham Elmahrek
> 
>


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