> On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java, line 141
> > <https://reviews.apache.org/r/26657/diff/1/?file=719676#file719676line141>
> >
> > Simirly as above I would suggest dropping the "Schema" word.
> >
> > In addition the "ForUpgrade" don't seem accurate as this method
> > implementation is not checking whether the repository is ready for upgrade.
> > It's checking that the repo is ready to be used. E.g. if this methods
> > return true then Sqoop will continue and start repository and won't run
> > upgrade as one would expect by the changed name.
>
> Veena Basavaraj wrote:
> I am not sure what the history of this method was. But as far the current
> implmentation is, it checks this , i,e if the version in the repository
> database is what the code states ( harcoded constant)
>
> @Override
> public boolean hasSuitableSchemaForUpgrade(Connection conn) {
> // TODO(jarcec): Verify that all structures are present (e.g.
> something like corruption validation)
> // NOTE: At this point is is just checking if the repo version
> matches the version
> // in the upgraded code
> return detectRepositoryVersion(conn) ==
> DerbyRepoConstants.LATEST_DERBY_REPOSITORY_VERSION;
> }
>
>
> if(!provider.getRepository().hasSuitableSchemaForUpgrade()) {
> throw new SqoopException(RepositoryError.REPO_0002);
> }
>
> this is how this method used today, and it throws an exception if the
> schema is not ready for upgrade. So What does "Suitable" really mean here.
> Might be good to think about it as well
I am reading this code more closely and even more confused about the naming and
functionality. so that upgrade happens first and then we throw an exception?
try {
provider = (RepositoryProvider) repoProviderClass.newInstance();
} catch (Exception ex) {
throw new SqoopException(RepositoryError.REPO_0001,
repoProviderClassName, ex);
}
provider.initialize(context);
if(!immutableRepository) {
LOG.info("Creating or update respository at bootup");
provider.getRepository().createOrUpgradeRepositorySchema();
}
if(!provider.getRepository().hasSuitableSchemaForUpgrade()) {
throw new SqoopException(RepositoryError.REPO_0002);
}
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26657/#review56443
-----------------------------------------------------------
On Oct. 13, 2014, 1:03 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26657/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 1:03 p.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> fix the upgrade logic for 1498 changes.
> While fixing the upgrade logic, added comments on how the upgrade logic works.
> -renamed test methods to reflect what they are upgrading and what they are
> infact testing.
> -renamed the internals to exactly mean "schema" upgrades, since we only
> allow/ do scheme upgrades the repository
> -Also updated the names on the repository api to clearly state the difference
> between upgrade and update operations on connector / driver.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 3ade247
> core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
> 97de893
> core/src/main/java/org/apache/sqoop/repository/Repository.java 95c7a4d
> core/src/main/java/org/apache/sqoop/repository/RepositoryManager.java
> c2f8505
> core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java
> e6e4760
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java
> 74e41df
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java
> 0f0f7c4
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
> 10a7b1a
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java
> cf6e657
>
> repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
> 56ea147
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java
> 9316687
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java
> fc95222
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestDriverHandling.java
> d597bd8
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java
> 260c2a9
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInternals.java
> 0eb9df4
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java
> 01a05b2
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java
> bbfe5bb
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java
> PRE-CREATION
>
> repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestSubmissionHandling.java
> 8402d8c
>
> Diff: https://reviews.apache.org/r/26657/diff/
>
>
> Testing
> -------
>
> yes
>
>
> Thanks,
>
> Veena Basavaraj
>
>
