----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42129/#review114087 -----------------------------------------------------------
test/src/main/java/org/apache/sqoop/test/infrastructure/InfraProvider.java (line 1) <https://reviews.apache.org/r/42129/#comment174876> we have a class called InfrastructureProvider and now the annotation InfraProvider. Can we clean up this naming to make things a little bit more clear? test/src/main/java/org/apache/sqoop/test/infrastructure/InfraProviderWrapper.java (line 24) <https://reviews.apache.org/r/42129/#comment174885> perhaps a string map would allow more flexibility and clarity with respect to what arguments are actually being passed through? test/src/main/java/org/apache/sqoop/test/infrastructure/Infrastructure.java (line 29) <https://reviews.apache.org/r/42129/#comment174893> why is this being changed here? were we ever actually using this annotation on a method? test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java (line 124) <https://reviews.apache.org/r/42129/#comment174898> would you mind explaining why we need alwaysRun now? test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java (line 147) <https://reviews.apache.org/r/42129/#comment174881> i think this is building a map of x -> (x, y). the duplication of x (the InfrastructureProvider) makes this code more complicated than it needs to be. is there a way that we can simplify things here? test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java (line 151) <https://reviews.apache.org/r/42129/#comment174906> do we still need this comment? test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java (line 214) <https://reviews.apache.org/r/42129/#comment174884> so the initParam from InfraProviderWrapper is only used if providerClass is SqoopInfrastructureProvider? it seems strange to me to do all the work of building the infrastructure to essentially pass this initParam string through and then handling it only when there is a special case. would it be possible to have a 1 argument string constructor in InfrastructureProvider that way we can always pass it through? test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java (line 218) <https://reviews.apache.org/r/42129/#comment174886> why the change? test/src/main/java/org/apache/sqoop/test/infrastructure/providers/SqoopInfrastructureProvider.java (line 40) <https://reviews.apache.org/r/42129/#comment174887> maybe 'clusterClass' would be a better name as the default cluster class is JettySqoopMiniCluster? test/src/main/java/org/apache/sqoop/test/infrastructure/providers/SqoopInfrastructureProvider.java (line 52) <https://reviews.apache.org/r/42129/#comment174911> after reading SqoopMiniClusterFactory.getSqoopMiniCluster i think we may run into issues if properties.getProperty(MINICLUSTER_CLASS_PROPERTY) is not null. perhaps changes need to be made to the factory class? test/src/test/java/org/apache/sqoop/integration/connectorloading/ConnectorClasspathIsolationTest.java (line 128) <https://reviews.apache.org/r/42129/#comment174888> can we revert this, it does not seem relevant just to clarify what exactly this code is doing because the description on the jira is a little bit confusing. currently, we start a sqoopinfrastructureprovider once per test suite. this code does not appear to change that behavior. what this code is doing is provide the infrastructure to pass arbitrary string arguments to infrastructure providers. this allows us to specify which sqoopminicluster class we start each suite with. we are currently only using those arguments for the sqoopinfrastructureprovider. why don't we just create subclasses of sqoopinfrastructureprovider for each different "type" of test sqoop server we want to run? - Abraham Fine On Jan. 12, 2016, 1:39 p.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42129/ > ----------------------------------------------------------- > > (Updated Jan. 12, 2016, 1:39 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Currently, SqoopInfrastructureProvider is started only once for test suite, > and the default SqoopMiniCluster is JettySqoopMiniCluster. With the different > test connectors, SqoopInfrastructureProvider should be started with different > SqoopMiniCluster. A restart feature should be added to support this. > > > Diffs > ----- > > test/src/main/java/org/apache/sqoop/test/infrastructure/InfraProvider.java > PRE-CREATION > > test/src/main/java/org/apache/sqoop/test/infrastructure/InfraProviderWrapper.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/infrastructure/Infrastructure.java > f3db7ad > test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java > becfa6b > > test/src/main/java/org/apache/sqoop/test/infrastructure/providers/SqoopInfrastructureProvider.java > 4d51ed6 > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > f071786 > > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/AppendModeTest.java > 6885525 > > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/FromHDFSToHDFSTest.java > c6ce1e8 > > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/HdfsIncrementalReadTest.java > 37306e2 > > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/InformalJobNameExecuteTest.java > 0e14b7a > > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/OutputDirectoryTest.java > e5e3d26 > test/src/test/java/org/apache/sqoop/integration/connector/hdfs/S3Test.java > c857699 > > test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java > ec9f733 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java > 9c0ee84 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromHDFSToRDBMSTest.java > 933bc08 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java > 7e66091 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java > 83012eb > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/PartitionerTest.java > e5e886e > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedRDBMSTest.java > 890fc10 > > test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromHDFSToKafkaTest.java > 5e349d1 > > test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java > 9ae1334 > > test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java > 10f3614 > > test/src/test/java/org/apache/sqoop/integration/connectorloading/ConnectorClasspathIsolationTest.java > 5b95631 > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java > 44f4a5b > > test/src/test/java/org/apache/sqoop/integration/server/InformalObjectNameTest.java > 5e204c8 > > test/src/test/java/org/apache/sqoop/integration/server/ShowJobInOrderTest.java > 49e26a2 > > test/src/test/java/org/apache/sqoop/integration/server/SubmissionWithDisabledModelObjectsTest.java > 9adebea > test/src/test/java/org/apache/sqoop/integration/server/VersionTest.java > 76002a5 > test/src/test/java/org/apache/sqoop/integration/server/rest/RestTest.java > 71384fb > > test/src/test/java/org/apache/sqoop/integration/tools/RepositoryDumpLoadToolTest.java > cce2c6c > > Diff: https://reviews.apache.org/r/42129/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
