Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Sqoop >> mail # dev >> Review Request 12774: Fix for SQOOP-1075: Persist framework version in repository


Copy link to this message
-
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in repository

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

common/src/main/java/org/apache/sqoop/model/MFramework.java
<https://reviews.apache.org/r/12774/#comment47493>

    It seems that we are not putting string constants into the model classes. It might be useful to stay consistent.

common/src/main/java/org/apache/sqoop/model/MFramework.java
<https://reviews.apache.org/r/12774/#comment47526>

    Is there a reason for this variable to be package private? I would suggest to have it private if not.

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

    I would expect that createOrUpdateFrameworkVersion will have at least two parameters - Connection to repository and the framework instance that we are working with. I know that the framework is a static singleton member, but the repository is designed in a way that it can process any framework structure (but still only one instance). We should not break the contract by directly using static variables.

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

    The table SQ_SYSTEM is internal table of the derby repository that should not depend on the some key stored in the model classes. I would suggest to create SYSKEY_SOMETHING entry in DerbyRepoConstants and use that.

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

    I would suggest to use the instance value rather than static member.

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

    Please use the version from mFramework rather than the static value.

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

    Nit: We are using brackets even in case of simple if statements, e.g.:
    
    if(resultSets == null) {
     return;
    }

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

    Nit: We are using brackets even in case of simple if statements, e.g.:
    
    if(stmts == null) {
     return;
    }

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

    This particular test code should not refer any constant from the code, but rather contain the copy of the key similarly as is done with the key for "version". The idea here is to intentionally break the test if the value in the code will be change to put a stress that this is backward incompatible change.
Jarcec

- Jarek Cecho
On July 19, 2013, 8:34 p.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12774/
> -----------------------------------------------------------
>
> (Updated July 19, 2013, 8:34 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1075
>     https://issues.apache.org/jira/browse/SQOOP-1075
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> A summary of changes:
> 1. MFramework: added fields, getter & setter method for version. Made changes to other methods to take version into account.
> 2. MConnector: since it extends MFramework, can use his version field and methods
> 3. FrameworkBean, FormSerialization: encode/decode version field to json
> 4. DerbyRepositoryHandler, DerbyRepoError: added private methods to persist/fetch framework version; update framework version when registering/updating framework