----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12774/#review24033 -----------------------------------------------------------
Hi Rangav, thank you very much for updating your patch. I do have additional notes: common/src/main/java/org/apache/sqoop/common/VersionInfo.java <https://reviews.apache.org/r/12774/#comment47828> The class VersionInfo contains information about the code itself (when it was compiled, by who, from what revision, ...). This information is stored in common module and as a result both client and server will have different values (provided that they are running on different versions). Putting here value specific for core module (e.g. for server) is very confusing as client might see different value then the server itself. I believe that the framework version should be stored inside the FrameworkManager class and that MFramework class should be just a container for it. common/src/main/java/org/apache/sqoop/model/MFramework.java <https://reviews.apache.org/r/12774/#comment47829> I do not quite understand the semantics of this method? What is the expected usage? It seems that it's suppose to return most up-to-date version that is available. However the value is taken from static class, so it will return different value on client and server provided that they are running on different versions. repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java <https://reviews.apache.org/r/12774/#comment47831> Nit: Rest of the framework is using property names in dot format, so I would suggest to rename this to "framework.version" rather than using the camel case "frameworkVersion". repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/12774/#comment47830> I'm assuming that there should be call getVersion() instead of getCurrentVersion()? Jarcec - Jarek Cecho On July 22, 2013, 8:03 p.m., Raghav Gautam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12774/ > ----------------------------------------------------------- > > (Updated July 22, 2013, 8:03 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 > ----- > > common/src/main/java/org/apache/sqoop/common/VersionInfo.java dcf522f > 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 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java > 607b8d5 > > 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 > >
