You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Abraham Elmahrek <ab...@cloudera.com> on 2015/04/26 21:00:43 UTC
Review Request 33570: SQOOP-1953: Sqoop2: Tomcat in suite
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33570/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-1953
https://issues.apache.org/jira/browse/SQOOP-1953
Repository: sqoop-sqoop2
Description
-------
commit 1250abe418f343ab0a2ffa010a2a15a97befb385
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Apr 17 17:34:15 2015 -0700
SQOOP-1953: Tomcat in suite
:100644 100644 bc2bec7... 6e5e038... M pom.xml
:100644 100644 98a60f6... a9502d2... M test/pom.xml
:100644 100644 4d27886... 5a6773d... M test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java
:100644 100644 6729cc7... 197dab4... M test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
:100644 100644 a687c16... 7ad3dc2... M test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgrade
:100644 100644 101b6ec... f0dd905... M test/src/test/resources/integration-tests-suite.xml
:000000 100644 0000000... 2856556... A test/src/test/resources/upgrade-tests-suite.xml
Diffs
-----
pom.xml bc2bec7
test/pom.xml 98a60f6
test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java 4d27886
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 6729cc7
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java a687c16
test/src/test/resources/integration-tests-suite.xml 101b6ec
test/src/test/resources/upgrade-tests-suite.xml PRE-CREATION
Diff: https://reviews.apache.org/r/33570/diff/
Testing
-------
Thanks,
Abraham Elmahrek
Re: Review Request 33570: SQOOP-1953: Sqoop2: Tomcat in suite
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33570/#review81645
-----------------------------------------------------------
Ship it!
Provided pre-commit hook passes.
- Jarek Cecho
On April 26, 2015, 9:05 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33570/
> -----------------------------------------------------------
>
> (Updated April 26, 2015, 9:05 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1953
> https://issues.apache.org/jira/browse/SQOOP-1953
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> commit 1250abe418f343ab0a2ffa010a2a15a97befb385
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Apr 17 17:34:15 2015 -0700
>
> SQOOP-1953: Tomcat in suite
>
> :100644 100644 bc2bec7... 6e5e038... M pom.xml
> :100644 100644 98a60f6... a9502d2... M test/pom.xml
> :100644 100644 4d27886... 5a6773d... M test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java
> :100644 100644 6729cc7... 197dab4... M test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
> :100644 100644 a687c16... 7ad3dc2... M test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgrade
> :100644 100644 101b6ec... f0dd905... M test/src/test/resources/integration-tests-suite.xml
> :000000 100644 0000000... 2856556... A test/src/test/resources/upgrade-tests-suite.xml
>
>
> Diffs
> -----
>
> pom.xml bc2bec7
> test/pom.xml 98a60f6
> test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java 4d27886
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 6729cc7
> test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java a687c16
> test/src/test/resources/integration-tests-suite.xml 101b6ec
> test/src/test/resources/upgrade-tests-suite.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33570/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 33570: SQOOP-1953: Sqoop2: Tomcat in suite
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33570/
-----------------------------------------------------------
(Updated April 26, 2015, 9:05 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1953
https://issues.apache.org/jira/browse/SQOOP-1953
Repository: sqoop-sqoop2
Description
-------
commit 1250abe418f343ab0a2ffa010a2a15a97befb385
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Apr 17 17:34:15 2015 -0700
SQOOP-1953: Tomcat in suite
:100644 100644 bc2bec7... 6e5e038... M pom.xml
:100644 100644 98a60f6... a9502d2... M test/pom.xml
:100644 100644 4d27886... 5a6773d... M test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java
:100644 100644 6729cc7... 197dab4... M test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
:100644 100644 a687c16... 7ad3dc2... M test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgrade
:100644 100644 101b6ec... f0dd905... M test/src/test/resources/integration-tests-suite.xml
:000000 100644 0000000... 2856556... A test/src/test/resources/upgrade-tests-suite.xml
Diffs (updated)
-----
pom.xml bc2bec7
test/pom.xml 98a60f6
test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java 4d27886
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 6729cc7
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java a687c16
test/src/test/resources/integration-tests-suite.xml 101b6ec
test/src/test/resources/upgrade-tests-suite.xml PRE-CREATION
Diff: https://reviews.apache.org/r/33570/diff/
Testing
-------
Thanks,
Abraham Elmahrek
Re: Review Request 33570: SQOOP-1953: Sqoop2: Tomcat in suite
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33570/
-----------------------------------------------------------
(Updated April 26, 2015, 8:19 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1953
https://issues.apache.org/jira/browse/SQOOP-1953
Repository: sqoop-sqoop2
Description
-------
commit 1250abe418f343ab0a2ffa010a2a15a97befb385
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Apr 17 17:34:15 2015 -0700
SQOOP-1953: Tomcat in suite
:100644 100644 bc2bec7... 6e5e038... M pom.xml
:100644 100644 98a60f6... a9502d2... M test/pom.xml
:100644 100644 4d27886... 5a6773d... M test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java
:100644 100644 6729cc7... 197dab4... M test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
:100644 100644 a687c16... 7ad3dc2... M test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgrade
:100644 100644 101b6ec... f0dd905... M test/src/test/resources/integration-tests-suite.xml
:000000 100644 0000000... 2856556... A test/src/test/resources/upgrade-tests-suite.xml
Diffs (updated)
-----
pom.xml bc2bec7
test/pom.xml 98a60f6
test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java 4d27886
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 6729cc7
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java a687c16
test/src/test/resources/integration-tests-suite.xml 101b6ec
test/src/test/resources/upgrade-tests-suite.xml PRE-CREATION
Diff: https://reviews.apache.org/r/33570/diff/
Testing
-------
Thanks,
Abraham Elmahrek
Re: Review Request 33570: SQOOP-1953: Sqoop2: Tomcat in suite
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On April 26, 2015, 8:07 p.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java, lines 65-68
> > <https://reviews.apache.org/r/33570/diff/1/?file=942028#file942028line65>
> >
> > Nit: I'm wondering why removing the more detailed descrption? Is it because it's no longer valid?
That's exactly right. The temporary directory was originally redefined on a per-method basis, now on a per-suite basis.
> On April 26, 2015, 8:07 p.m., Jarek Cecho wrote:
> > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java, lines 134-142
> > <https://reviews.apache.org/r/33570/diff/1/?file=942029#file942029line134>
> >
> > I do not particularly like this pattern when we are prohibit parent class to do something that it was designed to do. Do you think that it would make sense to split the TomcatTestCase to two classes:
> >
> > * TomcatTestCaseBase that will contain all variables/help methods but won't do any action. Then this DerbyRepositoryUpgradeTest class will inherit from the new base class.
> > * TomatTestCase that will inherit from Base and will add the automatic actions of starting/stopping required runners.
> >
> > I know that you're thinking about improving the integration test suite by using annotations to start/stop only those runners that are required, so perhaps my note is not so much relevant as this is just a "temporary" workaround.
I was thinking the exact same thing. I do plan on changing this drastically. Let's leave this and change it in the immediate future?
> On April 26, 2015, 8:07 p.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java, lines 86-91
> > <https://reviews.apache.org/r/33570/diff/1/?file=942028#file942028line86>
> >
> > I'm wondering why we are changing the visilibyt from private to protected?
> >
> > (I understand the addition of "static")
DerbyRepositoryUpgradeTest requires this for now.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33570/#review81633
-----------------------------------------------------------
On April 26, 2015, 7 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33570/
> -----------------------------------------------------------
>
> (Updated April 26, 2015, 7 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1953
> https://issues.apache.org/jira/browse/SQOOP-1953
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> commit 1250abe418f343ab0a2ffa010a2a15a97befb385
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Apr 17 17:34:15 2015 -0700
>
> SQOOP-1953: Tomcat in suite
>
> :100644 100644 bc2bec7... 6e5e038... M pom.xml
> :100644 100644 98a60f6... a9502d2... M test/pom.xml
> :100644 100644 4d27886... 5a6773d... M test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java
> :100644 100644 6729cc7... 197dab4... M test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
> :100644 100644 a687c16... 7ad3dc2... M test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgrade
> :100644 100644 101b6ec... f0dd905... M test/src/test/resources/integration-tests-suite.xml
> :000000 100644 0000000... 2856556... A test/src/test/resources/upgrade-tests-suite.xml
>
>
> Diffs
> -----
>
> pom.xml bc2bec7
> test/pom.xml 98a60f6
> test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java 4d27886
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 6729cc7
> test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java a687c16
> test/src/test/resources/integration-tests-suite.xml 101b6ec
> test/src/test/resources/upgrade-tests-suite.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33570/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 33570: SQOOP-1953: Sqoop2: Tomcat in suite
Posted by Jarek Cecho <ja...@apache.org>.
> On April 26, 2015, 8:07 p.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java, lines 86-91
> > <https://reviews.apache.org/r/33570/diff/1/?file=942028#file942028line86>
> >
> > I'm wondering why we are changing the visilibyt from private to protected?
> >
> > (I understand the addition of "static")
>
> Abraham Elmahrek wrote:
> DerbyRepositoryUpgradeTest requires this for now.
Can't we just create a setSqoopClient() method instead? Seems as cleaner approach then have this variable protected.
> On April 26, 2015, 8:07 p.m., Jarek Cecho wrote:
> > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java, lines 134-142
> > <https://reviews.apache.org/r/33570/diff/1/?file=942029#file942029line134>
> >
> > I do not particularly like this pattern when we are prohibit parent class to do something that it was designed to do. Do you think that it would make sense to split the TomcatTestCase to two classes:
> >
> > * TomcatTestCaseBase that will contain all variables/help methods but won't do any action. Then this DerbyRepositoryUpgradeTest class will inherit from the new base class.
> > * TomatTestCase that will inherit from Base and will add the automatic actions of starting/stopping required runners.
> >
> > I know that you're thinking about improving the integration test suite by using annotations to start/stop only those runners that are required, so perhaps my note is not so much relevant as this is just a "temporary" workaround.
>
> Abraham Elmahrek wrote:
> I was thinking the exact same thing. I do plan on changing this drastically. Let's leave this and change it in the immediate future?
Fine with me considering that this is one class.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33570/#review81633
-----------------------------------------------------------
On April 26, 2015, 8:19 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33570/
> -----------------------------------------------------------
>
> (Updated April 26, 2015, 8:19 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1953
> https://issues.apache.org/jira/browse/SQOOP-1953
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> commit 1250abe418f343ab0a2ffa010a2a15a97befb385
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Apr 17 17:34:15 2015 -0700
>
> SQOOP-1953: Tomcat in suite
>
> :100644 100644 bc2bec7... 6e5e038... M pom.xml
> :100644 100644 98a60f6... a9502d2... M test/pom.xml
> :100644 100644 4d27886... 5a6773d... M test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java
> :100644 100644 6729cc7... 197dab4... M test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
> :100644 100644 a687c16... 7ad3dc2... M test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgrade
> :100644 100644 101b6ec... f0dd905... M test/src/test/resources/integration-tests-suite.xml
> :000000 100644 0000000... 2856556... A test/src/test/resources/upgrade-tests-suite.xml
>
>
> Diffs
> -----
>
> pom.xml bc2bec7
> test/pom.xml 98a60f6
> test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java 4d27886
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 6729cc7
> test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java a687c16
> test/src/test/resources/integration-tests-suite.xml 101b6ec
> test/src/test/resources/upgrade-tests-suite.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33570/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 33570: SQOOP-1953: Sqoop2: Tomcat in suite
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33570/#review81633
-----------------------------------------------------------
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
<https://reviews.apache.org/r/33570/#comment132062>
Nit: I'm wondering why removing the more detailed descrption? Is it because it's no longer valid?
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
<https://reviews.apache.org/r/33570/#comment132063>
I'm wondering why we are changing the visilibyt from private to protected?
(I understand the addition of "static")
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
<https://reviews.apache.org/r/33570/#comment132064>
I'm wondering why we are changing the visilibyt from private to protected?
(I understand the addition of "static")
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java
<https://reviews.apache.org/r/33570/#comment132067>
I do not particularly like this pattern when we are prohibit parent class to do something that it was designed to do. Do you think that it would make sense to split the TomcatTestCase to two classes:
* TomcatTestCaseBase that will contain all variables/help methods but won't do any action. Then this DerbyRepositoryUpgradeTest class will inherit from the new base class.
* TomatTestCase that will inherit from Base and will add the automatic actions of starting/stopping required runners.
I know that you're thinking about improving the integration test suite by using annotations to start/stop only those runners that are required, so perhaps my note is not so much relevant as this is just a "temporary" workaround.
- Jarek Cecho
On April 26, 2015, 7 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33570/
> -----------------------------------------------------------
>
> (Updated April 26, 2015, 7 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1953
> https://issues.apache.org/jira/browse/SQOOP-1953
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> commit 1250abe418f343ab0a2ffa010a2a15a97befb385
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Apr 17 17:34:15 2015 -0700
>
> SQOOP-1953: Tomcat in suite
>
> :100644 100644 bc2bec7... 6e5e038... M pom.xml
> :100644 100644 98a60f6... a9502d2... M test/pom.xml
> :100644 100644 4d27886... 5a6773d... M test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java
> :100644 100644 6729cc7... 197dab4... M test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
> :100644 100644 a687c16... 7ad3dc2... M test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgrade
> :100644 100644 101b6ec... f0dd905... M test/src/test/resources/integration-tests-suite.xml
> :000000 100644 0000000... 2856556... A test/src/test/resources/upgrade-tests-suite.xml
>
>
> Diffs
> -----
>
> pom.xml bc2bec7
> test/pom.xml 98a60f6
> test/src/main/java/org/apache/sqoop/test/minicluster/TomcatSqoopMiniCluster.java 4d27886
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 6729cc7
> test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java a687c16
> test/src/test/resources/integration-tests-suite.xml 101b6ec
> test/src/test/resources/upgrade-tests-suite.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33570/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Abraham Elmahrek
>
>