----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26657/#review56443 -----------------------------------------------------------
Thank you for cutting the patch Veena! It seems that there are still some trailing whitespace issues, could you fix them for next version of the patch? core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java <https://reviews.apache.org/r/26657/#comment96752> Let's not call it "schema" as for example File repository (that don't exist right now) might not have schema, but just bunch of files and directories. What about just createOrUpgradeRepository() ? core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java <https://reviews.apache.org/r/26657/#comment96753> 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. core/src/main/java/org/apache/sqoop/repository/Repository.java <https://reviews.apache.org/r/26657/#comment96754> 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 repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java <https://reviews.apache.org/r/26657/#comment96755> Do we have a reason to keep it around? Like backward compatibility or something? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/26657/#comment96756> I'm wondering what are your thoughts about having multiple repositories at this level? I mean this is a derbo repy implementation, so this code will always deal with one single derby repo, correct? If sqoop will ever need more then one repository, then we will have multiple instances of this code pointing to different databases. repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/26657/#comment96757> This one seems as left over from debugging. I'm fine with keeping it in, but then we should make the comment more informative? We might do it in similar way as is line 2410 in the other log statement, so that we have the same log structure in the method? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java <https://reviews.apache.org/r/26657/#comment96764> Is this change necessary? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java <https://reviews.apache.org/r/26657/#comment96765> Nit: It seems that the intending is not aligned with the rest of the file (unless it's a review board displaying bug :)). repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java <https://reviews.apache.org/r/26657/#comment96766> Is this change intentional? repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java <https://reviews.apache.org/r/26657/#comment96767> 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. repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java <https://reviews.apache.org/r/26657/#comment96769> 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. repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java <https://reviews.apache.org/r/26657/#comment96770> Nit: Seems as unnecessary changes? Jarcec - Jarek Cecho 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 > >
