> 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 > > 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?
can I rephrase this in one sentence? is this simplify an api to say didUpgradeSucceed()? or veridyUpgradeSucceeded? - 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 > >
