----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30980/#review72452 -----------------------------------------------------------
In general, I'm +1 on the idea. I think to be safe it might be a good idea to push for https://issues.apache.org/jira/browse/SQOOP-910 as well. If we have no intention of switching to Jetty in the future, then this patch becomes wrong. I do think the issue you've brought up is sufficient reason to move to Jetty. Also, I'm wondering if it would be possible to have tomcat minicluster and jetty minicluster work in parallel until. It's preferred to continue testing with the tomcat minicluster since Sqoop2 runs on tomcat for now. test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java <https://reviews.apache.org/r/30980/#comment118507> Let's make this configurable so that strange dev. setups are supported. test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopRunner.java <https://reviews.apache.org/r/30980/#comment118509> This doesn't need to be volatile I think. "start" should be called from a single thread. test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopRunner.java <https://reviews.apache.org/r/30980/#comment118511> In our case, the port the server listens on will always be the same? Perhaps I'm misunderstanding something here. test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopRunner.java <https://reviews.apache.org/r/30980/#comment118508> Copy/paste error? Also, I'd add some doc describing what the arguments do here. test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopRunner.java <https://reviews.apache.org/r/30980/#comment118512> Seems unnecessary. The port is being passed to the constructor of this class. test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopRunner.java <https://reviews.apache.org/r/30980/#comment118510> I don't think this is used? test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java <https://reviews.apache.org/r/30980/#comment118514> We probably want to still be able to use tomcat here. test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java <https://reviews.apache.org/r/30980/#comment118513> Extra space? test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java <https://reviews.apache.org/r/30980/#comment118515> Let's continue to use tomcat for now. We should probably not be using inheritance here though. Maybe fix in a separate Jira. test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java <https://reviews.apache.org/r/30980/#comment118517> Let's pull these out into shared utils somewhere. test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java <https://reviews.apache.org/r/30980/#comment118521> Leave as tomcat test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java <https://reviews.apache.org/r/30980/#comment118520> Leave as tomcat test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java <https://reviews.apache.org/r/30980/#comment118519> Leave as tomcat test/src/test/java/org/apache/sqoop/integration/server/VersionTest.java <https://reviews.apache.org/r/30980/#comment118518> Leave as tomcat - Abraham Elmahrek On Feb. 13, 2015, 7:35 a.m., shen guoquan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30980/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2015, 7:35 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Currenly when the Tomcat Sqoop miniCluster started, it at firstly will need > to download the tomcat.tar.gz. When the Tomcat tar package has been > successfully downloaded, it will load the sqoop war package. That's is the > problem. For example, When the sentry want to do some integration tests with > sqoop, Sentry want to use the Tomcat Sqoop miniCluster to test, it must > download the sqoop war package in its project. It is not the correct way. The > correct method is that when Sentry want to do integration tests with Sqoop, > it only to do is add a maven dependency in the pom, then it can use a > independent sqoop miniCluster to test. > > > Diffs > ----- > > dist/pom.xml e0bf0d0 > pom.xml d48389e > server/pom.xml ee409cb > test/pom.xml f743d25 > > test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopRunner.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > 398a051 > test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java > a687c16 > test/src/test/java/org/apache/sqoop/integration/server/VersionTest.java > a54492e > > Diff: https://reviews.apache.org/r/30980/diff/ > > > Testing > ------- > > > Thanks, > > shen guoquan > >
