----------------------------------------------------------- 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 > 5. TestFrameworkHandling: added test - the test checks for current version of > framework, changes it to lower version updates the framework and checks the > version of framework once again > 6. TestSqoopClient, TestUtils, TestMFramework, FrameworkManager, > DerbyTestCase: added version parameter to the MFramework constructor call. > > > Diffs > ----- > > client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java 1778cf1 > common/src/main/java/org/apache/sqoop/json/FrameworkBean.java ad4753b > common/src/main/java/org/apache/sqoop/json/util/FormSerialization.java > 98768d6 > common/src/main/java/org/apache/sqoop/model/MConnector.java 1c2c422 > common/src/main/java/org/apache/sqoop/model/MFramework.java 694f022 > common/src/test/java/org/apache/sqoop/json/TestUtil.java b88d7a4 > common/src/test/java/org/apache/sqoop/model/TestMFramework.java a5366ca > core/src/main/java/org/apache/sqoop/framework/FrameworkManager.java ad6cd0f > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java > aeb7533 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > f717abf > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java > 677b0be > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestFrameworkHandling.java > 66611d4 > > Diff: https://reviews.apache.org/r/12774/diff/ > > > Testing > ------- > > I have added new tests. > All the unit tests are passing. > > > Thanks, > > Raghav Gautam > >
