You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Raghav Gautam <ra...@gmail.com> on 2013/07/19 22:34:49 UTC
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/
-----------------------------------------------------------
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
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Jarek Cecho <ja...@apache.org>.
> On July 22, 2013, 3:54 p.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/model/MFramework.java, line 37
> > <https://reviews.apache.org/r/12774/diff/1/?file=323718#file323718line37>
> >
> > Is there a reason for this variable to be package private? I would suggest to have it private if not.
>
> Raghav Gautam wrote:
> version is a field that makes sense even for the MConnector (child class of MFramework), that is why I did not make it private.
Thank you for the explanation Raghav, the explanation make sense to me.
On July 22, 2013, 3:54 p.m., Raghav Gautam wrote:
> > Jarcec
>
> Raghav Gautam wrote:
> Uploading new patch.
Thank you!
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12774/#review23599
-----------------------------------------------------------
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
>
>
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Raghav Gautam <ra...@gmail.com>.
> On July 22, 2013, 8:54 a.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/model/MFramework.java, line 37
> > <https://reviews.apache.org/r/12774/diff/1/?file=323718#file323718line37>
> >
> > Is there a reason for this variable to be package private? I would suggest to have it private if not.
version is a field that makes sense even for the MConnector (child class of MFramework), that is why I did not make it private.
On July 22, 2013, 8:54 a.m., Raghav Gautam wrote:
> > Jarcec
Uploading new patch.
- Raghav
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12774/#review23599
-----------------------------------------------------------
On July 19, 2013, 1: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, 1: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
>
>
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Venkat Ranganathan <n....@live.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12774/#review23564
-----------------------------------------------------------
As I have done most of the preliminary reviews of the patch before, I only have two nits below. I will let others review
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestFrameworkHandling.java
<https://reviews.apache.org/r/12774/#comment47463>
This can be a static String along with other such declarations
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestFrameworkHandling.java
<https://reviews.apache.org/r/12774/#comment47464>
This can be a static final String as well
- Venkat Ranganathan
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
>
>
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Raghav Gautam <ra...@gmail.com>.
> On July 27, 2013, 10:52 a.m., Jarek Cecho wrote:
> > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java, line 888
> > <https://reviews.apache.org/r/12774/diff/2/?file=324963#file324963line888>
> >
> > I'm assuming that there should be call getVersion() instead of getCurrentVersion()?
This is a no-op, removing.
- Raghav
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12774/#review24033
-----------------------------------------------------------
On July 22, 2013, 1: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, 1: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
>
>
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12774/#review24180
-----------------------------------------------------------
Hi Raghav,
thank you for incorporating all the feedback! I've applied the patch and it seems that I'm getting compilation failures (for example at file TestJdbcRepository) due to the changed constructor of MFramework structure. Would you mind fixing those?
core/src/main/java/org/apache/sqoop/framework/FrameworkManager.java
<https://reviews.apache.org/r/12774/#comment48002>
Nit: Looking at how the JDBC Generic connector is handling the version, it seems that is actually using the release version from VersionInfo.getVersion(). I don't think that we have to be necessary consistent here as it's different subsystem, so I'm just sharing the idea for consideration:
https://github.com/apache/sqoop/blob/sqoop2/connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnector.java#L58
Jarcec
- Jarek Cecho
On July 29, 2013, 8:25 p.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12774/
> -----------------------------------------------------------
>
> (Updated July 29, 2013, 8:25 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/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
>
>
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Raghav Gautam <ra...@gmail.com>.
> On July 29, 2013, 6 p.m., Jarek Cecho wrote:
> > Please attach the final patch to the JIRA, so that I can commit it!
Done. Thanks for reviewing !!
- Raghav
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12774/#review24189
-----------------------------------------------------------
On July 29, 2013, 3:57 p.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12774/
> -----------------------------------------------------------
>
> (Updated July 29, 2013, 3:57 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 978789b
> 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
> core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 247e165
> 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 455eb64
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 771673d
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java b766b09
> 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
>
>
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12774/#review24189
-----------------------------------------------------------
Ship it!
Please attach the final patch to the JIRA, so that I can commit it!
- Jarek Cecho
On July 29, 2013, 10:57 p.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12774/
> -----------------------------------------------------------
>
> (Updated July 29, 2013, 10:57 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 978789b
> 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
> core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 247e165
> 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 455eb64
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 771673d
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java b766b09
> 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
>
>
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Raghav Gautam <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12774/
-----------------------------------------------------------
(Updated July 29, 2013, 3:57 p.m.)
Review request for Sqoop.
Changes
-------
Hi Jarek, I have rebased my patch and unit tests look good. Sorry for the trouble.
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 (updated)
-----
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 978789b
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
core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 247e165
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 455eb64
repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java 771673d
repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java b766b09
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
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Raghav Gautam <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12774/
-----------------------------------------------------------
(Updated July 29, 2013, 1:25 p.m.)
Review request for Sqoop.
Changes
-------
Updating patch according to review comments.
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 (updated)
-----
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/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
Re: Review Request 12774: Fix for SQOOP-1075: Persist framework version in
repository
Posted by Raghav Gautam <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12774/
-----------------------------------------------------------
(Updated July 22, 2013, 1:03 p.m.)
Review request for Sqoop.
Changes
-------
Updated patch according to the review comments.
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 (updated)
-----
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