sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 28373: SQOOP-1783 Sqoop2: Create derby integration upgrade tests
Date Mon, 24 Nov 2014 23:52:37 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message