----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10964/#review22752 -----------------------------------------------------------
Hi Abe, thank you very much for rebasing this patch after all the changes that happened on the sqoop2 branch, greatly appreciated! It seems that current implementation is forcing the minicluster to be run either for all test cases or to none. We have multiple types of test cases, some of them like VersionTest are not executing any mapreduce jobs and thus using the minicluster for them by default seems to be unnecessary waste of resources. What about creating the "Hadoop Mini Cluster" as a first level citizen in the test module similarly as is the SqoopMiniCluster implementation that the tests cases are simply reusing? We can create abstract HadoopMiniCluster class with two concrete implementations for LocalJobRunner and MiniCluster. Each test case class could then choose default implementation - for example TomcatTestCase would use "LocalJobRunner" by default (and transitively also VersionTest) whereas the ConnectorTestCase which is mostly running mapreduce jobs would by default use the "MiniCluster". We could also do it overridable by system property, so that user can force the implementation as he seems fit (s imilarly as DatabaseProviderFactory is doing for database providers). This solution would be very easily extensible with running on real cluster by creating different concrete implementation. test/src/main/java/org/apache/sqoop/test/asserts/HdfsAsserts.java <https://reviews.apache.org/r/10964/#comment46451> Nit: Can we remove this deprecated comment? test/src/main/java/org/apache/sqoop/test/asserts/HdfsAsserts.java <https://reviews.apache.org/r/10964/#comment46452> Nit: Trailing whitespace characters. test/src/main/java/org/apache/sqoop/test/asserts/HdfsAsserts.java <https://reviews.apache.org/r/10964/#comment46531> Is this removal intentional? test/src/main/java/org/apache/sqoop/test/testcases/HadoopTestCase.java <https://reviews.apache.org/r/10964/#comment46534> This file is missing license header. test/src/main/java/org/apache/sqoop/test/testcases/HadoopTestCase.java <https://reviews.apache.org/r/10964/#comment46532> Nit: Trailing white space characters. test/src/main/java/org/apache/sqoop/test/testcases/HadoopTestCase.java <https://reviews.apache.org/r/10964/#comment46535> Do we need to restart the minicluster for each individual test method? Would @BeforeClass be sufficient here? test/src/main/java/org/apache/sqoop/test/utils/HdfsUtils.java <https://reviews.apache.org/r/10964/#comment46533> Nit: Trailing white space characters. Jarcec - Jarek Cecho On July 4, 2013, 7:59 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10964/ > ----------------------------------------------------------- > > (Updated July 4, 2013, 7:59 p.m.) > > > Review request for Sqoop and Jarek Cecho. > > > Bugs: SQOOP-927 > https://issues.apache.org/jira/browse/SQOOP-927 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit ff7dba55a09dd7789a34136233000c625759e583 > Author: Abraham Elmahrek <[email protected]> > Date: Fri Apr 26 15:10:24 2013 -0700 > > SQOOP-927 Sqoop2: Integration: Mapreduce specific tests should be running > on MiniCluster > > Handle MiniDFSCluster and MiniMRClientCluster on own. > > Set yarn.application.classpath to get over classpath errors. > Set to use fair scheduler. > > :100644 100644 0abbb18... f09704b... M pom.xml > :100644 100644 6eb3184... a4c2a5b... M > test/src/test/java/org/apache/sqoop/integration/TomcatTestCase.java > :100644 100644 0f48a8b... 758c885... M > test/src/test/java/org/apache/sqoop/integration/server/VersionTest.java > > > Diffs > ----- > > pom.xml 8785e01000cc6e7d5a74ffeb96b83d236a657df8 > test/src/main/java/org/apache/sqoop/test/asserts/HdfsAsserts.java > 056e6124b918ce5821c389e388a0cdfa68fd7fc0 > test/src/main/java/org/apache/sqoop/test/testcases/HadoopTestCase.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java > ca77e64486d80b38306b5b30e185e6278ad7eaf1 > test/src/main/java/org/apache/sqoop/test/utils/HdfsUtils.java > 95dd177a11c5d822e35ca54e5d93785f0a40fbfc > > Diff: https://reviews.apache.org/r/10964/diff/ > > > Testing > ------- > > Ran integration tests without and with miniclusters. > Currently need to use both miniclusters or neither for tests to work. > > > Thanks, > > Abraham Elmahrek > >
