> On Nov. 24, 2014, 11:15 p.m., Veena Basavaraj wrote: > >
Thank you for your valuable feedback Veena. > On Nov. 24, 2014, 11:15 p.m., Veena Basavaraj wrote: > > test/src/main/java/org/apache/sqoop/test/minicluster/SqoopMiniCluster.java, > > line 214 > > <https://reviews.apache.org/r/28373/diff/1/?file=773799#file773799line214> > > > > rename this to connectorUpgradeProperty ( configuration is overloaded > > term in sqoop_ > > > > It is not a connector configuration. I understand why you have named it > > this way since there are other places, if you can possiblu change all > > others its good. It seems to me that your comment is not for this line, but for the line above, correct? I'm assuming that you would prefer to rename the newly introduced method getConnectorConfiguration()? If so then, I would suggest to use connectorManagerConfiguration() as that is more descriptive. Even though that the default method implementation is configuring only the upgrade, other ConnectorManager configuration properties could be specified there. > On Nov. 24, 2014, 11:15 p.m., Veena Basavaraj wrote: > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java, > > line 46 > > <https://reviews.apache.org/r/28373/diff/1/?file=773803#file773803line46> > > > > Will we add a test per repository? in that case please create a ticket > > for post gres since that is important to have before we call the post gres > > repo work done. > > > > > > Please rename this class to reflect the below, > > > > It is not testing repository schema upgrade, it should be exercising > > the upgradeConnector nd upgradeDriver code in the Respository.java. > > > > So please plug the word data into. > > > > Please add these comments explaining what this test case does, so > > someone new can really understand it > > > > DerbyRepoDataUpgrade seems more apt. Yes, I would assume that we will end up having different tests for different repository implementations. Considering that different repository implementations will have different physical structure and requirementes, I believe that it's expected. E.g. I'm assuming that for PostgreSQL we will have to do pg_dump instead of simply targzing the repository structures. We can't add PostgreSQL upgrade tests, because PostgreSQL haven't been release yet and hence there is nothing to upgrade from. I'll however open subsequent JIRA to not forget on this one. I would also like to see a step in our "How to release wiki page" that will request creation of such JIRAs post every release, so that we will have new tests going forward as well. This test actually exercise both schema changes and data changes. The idea is to have a snapshot of previous version and verify that I can get from the previous version all the way to current state. And this might (and will) include both structural changes and data changes. Hence If you don't mind, I would like to keep the name as it is. > On Nov. 24, 2014, 11:15 p.m., Veena Basavaraj wrote: > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java, > > line 126 > > <https://reviews.apache.org/r/28373/diff/1/?file=773803#file773803line126> > > > > can we have any more assert here to say that the upgrade actually > > kicked off, some version assertion? I was thinking about that and sadly I currently don't have a good way how to directly detect whether the upgrade happened. We know that it happened only based on the invaritants that are true when you get to the testPostUpgrade() method call. The invariant is that the server is up and running and as a result if we started with older repository version, it had to be upgraded (otherwise the server would fail to start). I recognize that this is not the ideal state, but I don't have anything better available at the moment. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28373/#review62894 ----------------------------------------------------------- On Nov. 23, 2014, 2:24 a.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28373/ > ----------------------------------------------------------- > > (Updated Nov. 23, 2014, 2:24 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1783 > https://issues.apache.org/jira/browse/SQOOP-1783 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > I've added automated test as suggested on the JIRA. > > You need to put archive derby-repository-1.99.4.tar.gz that is attached to > JIRA to path > test/src/test/resources/repository/derby/derby-repository-1.99.4.tar.gz in > order to verify this patch. > > > Diffs > ----- > > pom.xml e626c7d > test/pom.xml cafa250 > test/src/main/java/org/apache/sqoop/test/minicluster/SqoopMiniCluster.java > 4322b1c > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java > 5e1d564 > test/src/main/java/org/apache/sqoop/test/utils/CompressionUtils.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_4UpgradeTest.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java > PRE-CREATION > test/src/test/resources/repository/derby/derby-repository-1.99.4.tar.gz > PRE-CREATION > > Diff: https://reviews.apache.org/r/28373/diff/ > > > Testing > ------- > > The new test is passing! > > > Thanks, > > Jarek Cecho > >
