sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Abraham Fine" <abef...@cloudera.com>
Subject Re: Review Request 42129: SQOOP-2782: Sqoop2: improvement SqoopInfrastructureProvider to support restart with different SqoopMiniCluster
Date Wed, 13 Jan 2016 00:13:31 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message