> 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); > }
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? > On Oct. 13, 2014, 9:20 p.m., Jarek Cecho wrote: > > core/src/main/java/org/apache/sqoop/repository/Repository.java, lines 63-66 > > <https://reviews.apache.org/r/26657/diff/1/?file=719678#file719678line63> > > > > If we want to be that specific then we should also state that this > > method will be called during upgrade tool: > > > > > > https://github.com/apache/sqoop/blob/sqoop2/docs/src/site/sphinx/Upgrade.rst#upgrading-server-using-upgrade-tool > > Veena Basavaraj wrote: > Sure we should add it, I did not know it existed:) > > BTW, this link to upgrade tool is broken on git > > https://github.com/apache/sqoop/blob/sqoop2/docs/src/site/sphinx/Upgrade.rst#upgrading-server-using-upgrade-tool > > Veena Basavaraj wrote: > > https://github.com/apache/sqoop/blob/sqoop2/docs/src/site/sphinx/Tools.html#upgrade > I meant this link. That is actually "fine" as the documentation is not meant to work on Github :) It's part of the source code and it's just Github who is trying to look smart and present it in better form. The eventual place for the docs is in the release tarbal and our public website: http://sqoop.apache.org/docs/1.99.3/index.html Where all the links work correctly. > On Oct. 13, 2014, 9:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java, > > lines 24-28 > > <https://reviews.apache.org/r/26657/diff/1/?file=719681#file719681line24> > > > > Do we have a reason to keep it around? Like backward compatibility or > > something? > > Veena Basavaraj wrote: > yes, the test code the way it is written requires it. I see, make sense then. > On Oct. 13, 2014, 9:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java, > > lines 20-55 > > <https://reviews.apache.org/r/26657/diff/1/?file=719686#file719686line20> > > > > Is this change intentional? > > Veena Basavaraj wrote: > yes it is, it is always hard to do merges with every single import added. > > I would like to understand if this is performance concern/ nitpick or is > there some standards we follow for the import order. I did not see it > documented anywhere, would be nice to have a pointer to the code style > expected. I usually don't like asterisks import as they are masking what is actually used in the module. In this particular case where it's obviously that we're trying to get all the various queries, I don't mind as it's just following the very close relationship between those two classes. I was asking to understand whether it's intentional change or some automatic IDE change as there are IDEs that are automatically mergin multiple import statements together. > On Oct. 13, 2014, 9:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java, > > lines 96-97 > > <https://reviews.apache.org/r/26657/diff/1/?file=719686#file719686line96> > > > > We should keep this constant here as well - again it's intentionally > > duplicated here, so that the developer is intentionally changing it on two > > places to realize need to do the upgrade. > > Veena Basavaraj wrote: > why would we have duplication like this in the code? Test code usually > uses the same constants. This is asking for trouble to have duplicate code in > test, which I have already seen is true in many places in the sqoop code, > refactoring breaks it We are sharing pretty much almost all the constants already. We have few exceptions where we believe that devs should double check before changing them to ensure that we won't break important functionality such as upgrades. One such example is here - we want to allow dev to just change the number and get away with it, we want the test to break if the change is not intentional. And if it's intentional, then the dev have to provide an upgrade path which will require much more changes and will render this one small constant insignificant. > On Oct. 13, 2014, 9:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java, > > lines 205-206 > > <https://reviews.apache.org/r/26657/diff/1/?file=719686#file719686line205> > > > > The reason why we have the code duplicated here, is that if somebody > > will unintentionally change the code in DerbyRepositoryHandler, then all > > tests will start breaking and he will realize that the change has been > > invalid as it requires an upgrade path. Hence unless we will somehow > > accomplish this goal differently, I would prefer to keep this particular > > code duplication. > > Veena Basavaraj wrote: > There are other ways to handle such things, than duplicating code, I am > happy to create a ticket for this and fix it the right way, instead of > debating. I really dont like how copy paste is considered a solution for > things. > > Veena Basavaraj wrote: > BTW, when I made changed for SQOOP-1498, I did not do the upgrade logic > the right way, neither the tests broke nor did I find any documentation on > the right way to upgrade. So I am not sure if you still think the copy paste > magic i helping guard a developer to not break functionality. I'm open to use better approach if we have any. I'm not sayin that this is 100% solution, but it's also non 0%. Feel free to create follow up JIRA. > On Oct. 13, 2014, 9:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java, > > lines 20-56 > > <https://reviews.apache.org/r/26657/diff/1/?file=719689#file719689line20> > > > > Nit: Seems as unnecessary changes? > > Veena Basavaraj wrote: > same as above, is there a import order imposed so I can use that as a > standard in the IDE. If we have more developers contributing to the code > base, projects use a style guide as a standard to avoid debating on such > things:). Time well spent on real issues. I don't think that we have any style guide with regards to the imports right now. I'm open to include it, but I would be sceptical to apply it everywhere immediatelly. My concern here will pretty much follow what I've mentioned elsewhere - this code will make the following commands either unuseable or troublesome: 1) git blame 2) git cherrypick Where both commands are very important in long term project where multiple developers are working on the code base and trying to figure out what/when/why. - 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 > >
