> On Oct. 13, 2014, 9: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 > > Veena Basavaraj wrote: > 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); > } > > Jarek Cecho wrote: > The method haveSuitableInternals() is suppose to return true if and only > if the on disk/database structures are in state that the Repository can > operate on them - e.g. that there is no corruption and that those structures > don't need any changes (~any update). The way Derby Repository is > implementing this semantics is to ask a question whether the database have > the same schema version as the code expects. Based on the semantics this > method should actually return false if the repository requires an upgrade. > > The last code example is generic Sqoop Server code (not a Derby > repository implementation) that is calling this method to verify that > repository backend (database, on disk structures, simply "internals) are in a > state that the repo can immediatelly start working with them and throws an > exception if that is not the case as Sqoop Server can't work without a > working repository. > > Does that make more sense? > > Veena Basavaraj wrote: > can I rephrase this in one sentence? is this simplify an api to say > didUpgradeSucceed()? or veridyUpgradeSucceeded?
Not really, because the upgrade might not be allowed at all. Check the code you've pasted - the method createOrUpgradeRepositorySchema() will be called if and only if immutableRepository is set to false. However hasSuitableSchemaForUpgrade() will be call in all cases. Hence the hasSuitableSchemaForUpgrade() is really suppose to find out whether the current internal are in a state that the code can work with them, regardless whether there was any upgrade or not. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26657/#review56443 ----------------------------------------------------------- On Oct. 13, 2014, 8: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, 8: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 > >
