sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Veena Basavaraj" <vbasava...@cloudera.com>
Subject Re: Review Request 28373: SQOOP-1783 Sqoop2: Create derby integration upgrade tests
Date Tue, 25 Nov 2014 00:16:18 GMT


> On Nov. 24, 2014, 3: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?
> 
> Jarek Cecho wrote:
>     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.

Please add a todo and follow up to do so, it is a good start but still needs some more work
to guard against bad code


> On Nov. 24, 2014, 3: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.
> 
> Jarek Cecho wrote:
>     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.

I actuall said please create a ticket for post gres. Not a test.!! So filing a JIRA as a sub
task of the post gres task would be not hard. https://issues.apache.org/jira/browse/SQOOP-1523

If it is testing both, lets make it explicit in a comment and or the name of the class. 

Quoting from the kafka guide:

http://kafka.apache.org/coding-guide.html

Clear code is preferable to comments. When possible make your naming so good you don't need
comments. When that isn't possible comments should be thought of as mandatory, write them
to be read.


> On Nov. 24, 2014, 3: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.
> 
> Jarek Cecho wrote:
>     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.

sure Manager works.


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28373/#review62894
-----------------------------------------------------------


On Nov. 22, 2014, 6:24 p.m., Jarek Cecho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28373/
> -----------------------------------------------------------
> 
> (Updated Nov. 22, 2014, 6:24 p.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