You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Colin Ma <ju...@intel.com> on 2016/01/11 07:15:10 UTC
Review Request 42129: SQOOP-2782: Sqoop2: improvement
SqoopInfrastructureProvider to support restart with different
SqoopMiniCluster
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42129/
-----------------------------------------------------------
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/ConnectorClasspathTestCase.java 6db1db8
test/src/main/java/org/apache/sqoop/test/testcases/ConnectorLoadingTestCase.java PRE-CREATION
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/BlacklistedConnectorTest.java 66c20a1
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
Re: Review Request 42129: SQOOP-2782: Sqoop2: improvement
SqoopInfrastructureProvider to support restart with different
SqoopMiniCluster
Posted by Abraham Fine <ab...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 42129: SQOOP-2782: Sqoop2: improvement
SqoopInfrastructureProvider to support restart with different
SqoopMiniCluster
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
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 (updated)
-----
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