----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10964/#review22919 -----------------------------------------------------------
Hi Abe, thank you very much for incorporating all my suggestions. I think that we are on track to get it committed very soon! pom.xml <https://reviews.apache.org/r/10964/#comment46678> Do you think that it would make sense to bump the version up to the latest 2.0.5-alpha release? test/src/main/java/org/apache/sqoop/test/hadoop/HadoopCluster.java <https://reviews.apache.org/r/10964/#comment46668> Please provide JavaDoc describing purpose of this abstract class. test/src/main/java/org/apache/sqoop/test/hadoop/HadoopCluster.java <https://reviews.apache.org/r/10964/#comment46667> Nit: This comment seems to be copied from the TomcatTestCase, but I'm not sure that it's applicable here. The method setTemporaryPath() can set any arbitrary directory so the pattern might not be accurate (the class itself can be used outside TomcatTestCase). test/src/main/java/org/apache/sqoop/test/hadoop/HadoopCluster.java <https://reviews.apache.org/r/10964/#comment46692> Nit: Please improve the java doc a bit by for example stating when this method will be called or what are the allowed/disallowed operations. It seems that the expected usage to call this method only once prior running start() method. test/src/main/java/org/apache/sqoop/test/hadoop/HadoopClusterFactory.java <https://reviews.apache.org/r/10964/#comment46666> I would suggest to start the property name with prefix "sqoop." to avoid any possible conflict with Hadoop property of the same name. test/src/main/java/org/apache/sqoop/test/hadoop/HadoopMiniCluster.java <https://reviews.apache.org/r/10964/#comment46691> The getDataDir seems to be overloaded. Here it's used for local files that are generated by the MiniClusters, however we are also using it for the HDFS files. I would recommend to distinguish those two usages. test/src/main/java/org/apache/sqoop/test/hadoop/HadoopStandaloneCluster.java <https://reviews.apache.org/r/10964/#comment46670> Nit: I'm afraid that the word "cluster" in the name can be quite confusing as this is not using any cluster at all. Would it be possible to rename it to something like LocalMode or LocalRunner? test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java <https://reviews.apache.org/r/10964/#comment46693> I think that we should be able to put the HadoopCluster initialization into method annotated with @BeforeClass and override this method in child if necessary. This way we should save some time bootstrapping the Minicluster for test cases that have more then one test method. test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java <https://reviews.apache.org/r/10964/#comment46671> Nit: This change do not seem to be necessary. test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java <https://reviews.apache.org/r/10964/#comment46681> I think that the main temporary path should be owned by this class because we are storing there a lot of information required for the Sqoop Server itself (logs, metastore, configuration, ....). Those files exists outside of the mapreduce/hdfs. test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java <https://reviews.apache.org/r/10964/#comment46684> I don't think that we should be cleaning up all the directories after the test. The directory created on local file system will be cleaned up automatically by either next test or maven itself. The question is what to do with the directories on HDFS in case of minicluster or future real cluster. I would personally vote to not remove them as such action will complicate investigation of non deterministic issues and will most likely lead to a disabling the removal anyway. test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java <https://reviews.apache.org/r/10964/#comment46683> I would suggest to distinguish between main temporary directory for local files (configuration, logs, ...) and temporary directory for HDFS files (on minicluster). The local directory should remain inside the "target" directory of maven structures, so that it will be cleaned up on "mvn clean". The HDFS directory on Local Mode should remain inside the "target" dir as well. I think that in case of a mini cluster the path can be arbitrary as entire file system is recreated every time. However we will have to remain the directory separation for each test method in case that we will be reusing the MiniCluster for multiple tests. test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java <https://reviews.apache.org/r/10964/#comment46673> Nit: The method reorder do not seem to be necessary. test/src/main/java/org/apache/sqoop/test/utils/HdfsUtils.java <https://reviews.apache.org/r/10964/#comment46674> Nit: Please remove the comment as it's no longer applicable. test/src/main/java/org/apache/sqoop/test/utils/HdfsUtils.java <https://reviews.apache.org/r/10964/#comment46675> Nit: Is the logger removal intentional? test/src/main/java/org/apache/sqoop/test/utils/HdfsUtils.java <https://reviews.apache.org/r/10964/#comment46680> Nit: This method seems to be joining multiple path fragments into one single path. Maybe it would be worth to rename the method and javadoc to reflect that? Jarcec - Jarek Cecho On July 8, 2013, 8:47 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10964/ > ----------------------------------------------------------- > > (Updated July 8, 2013, 8:47 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/hadoop/HadoopCluster.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/hadoop/HadoopClusterFactory.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/hadoop/HadoopMiniCluster.java > PRE-CREATION > > test/src/main/java/org/apache/sqoop/test/hadoop/HadoopStandaloneCluster.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > 6aeadd4e54e47e5644270b15813b2d9c4cedc059 > 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 > >
