> 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? > > Jarek Cecho wrote: > 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. > > Veena Basavaraj wrote: > Fair point, that this method is not veifying the upgrade happened or not > and has different responsibility. > > But I am assuming that it should be optimally only called when some > changes were applied to the repository. It does seem unnecessary to call when > we know for sure the repository did not get created/ updated. > > Second, to answer if the repository is in a good state or not, why do we > need another api.? As you stated earlier... > >>> It's checking that the repo is ready to be used. > > Should not the creation/ upgrading code, throw an exception immediately > if something went wrong while doing the database operation.? > > Do we expect repository implementation to silently fail? > > > > Aside: > > Also, you seem to use repository in some places when you actually mean > repository manager. This confused me a lot. > >>are in a state that the repo can immediatelly start working with them
I think that I see now why we are not on the same page. I think that you feel that the method haveSuitableInternals() do have sense only after upgrade as we know for sure that repository structures can't get changed otherwise, right? If so, then I was missing this point of view :) Based on my experience, this is actually not the case - our users can upgrade the code without upgradeing the repository backend, can recover backup of the backend, can switch from master to slave in replication scenarios and all those things can lead to a situation when your internals has changed without us knowing it. I think that the problem is relatively insignificant with Derby, but I would assume that it will start being real trouble when we add support for Oracel/Postgre/MySQL. Hence I do feel that we should run the method haveSuitableInternals() on every start to verify that repository backend (db, files, ...) are in state that we are expecting as we generally don't own the repository structures and those can change without us knowing it. - 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 > >
