sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request: Review request for SQOOP-1035 "Add MS Sqoop Connecter tests to repo"
Date Sun, 09 Jun 2013 16:23:47 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10987/#review21151
-----------------------------------------------------------


Hi Shuaishuai,
thank you for this significant effort! I do have couple of high level comments:

1) Thank you for putting the new tests into new "manager" package. Do you think that it would
make sense to go even further and put the MS SQL specific tests into "manager/sqlserver" package?
I'm assuming that we will add more tests for other connectors in the future as well, so this
might help to have the packages clean.

2) Can you register all the new tests in ThirdParty test suite? (https://github.com/apache/sqoop/blob/trunk/src/test/com/cloudera/sqoop/ThirdPartyTests.java)
I know that existing SQL Server tests are not there, but I believe that they should.



conf/MSTest.properties
<https://reviews.apache.org/r/10987/#comment43920>

    The top level conf/ directory should be used only for code that is mean to be used for
the actual product, not for the tests. Would it be feasible to move it somewhere else? For
example to testdata/ or src/test?
    
    Also this file is missing Apache License.



conf/MSTest.properties
<https://reviews.apache.org/r/10987/#comment44700>

    Rest of the project is using lowercased properties separating logical parts using dot.
For example JDBC URL is in existing SQLServerManagerImportManualTest specified via following
property:
    
    sqoop.test.sqlserver.connectstring.host_url
    
    I think that it would make sense to stay consistent. Also I would suggest to reuse properties
that are already used, such the connection string above.



conf/MSTest.properties
<https://reviews.apache.org/r/10987/#comment44696>

    Sqoop is primary supported on linux, so please change all defaults from Windows domains
to a linux paths. Also I would strongly advise to use relative paths rather then absolute
ones.



src/test/com/cloudera/sqoop/manager/SQLServerManagerImportManualTest.java
<https://reviews.apache.org/r/10987/#comment44697>

    Nit: Is this isolated change necessary?



src/test/org/apache/sqoop/manager/MSSQLTestData.java
<https://reviews.apache.org/r/10987/#comment43921>

    Can we add javadoc explaining purpose of this class?



src/test/org/apache/sqoop/manager/MSSQLTestData.java
<https://reviews.apache.org/r/10987/#comment44698>

    Can we add comments explaining the enum constants with their purpose?



src/test/org/apache/sqoop/manager/MSSQLTestData.java
<https://reviews.apache.org/r/10987/#comment44699>

    Nit: Is the main method artifact from previous development?



src/test/org/apache/sqoop/manager/MSSQLTestDataFileParser.java
<https://reviews.apache.org/r/10987/#comment43922>

    Nit: This seems to artifact from development.



src/test/org/apache/sqoop/manager/MSSQLTestUtils.java
<https://reviews.apache.org/r/10987/#comment43923>

    Is there a linux equivalent for these commands?



src/test/org/apache/sqoop/manager/SQLServerDatatypeImportSequenceFileManualTest.java
<https://reviews.apache.org/r/10987/#comment43924>

    Nit: Indentation seems to be off here.



testdata/DatatypeTestData-export-lite.txt
<https://reviews.apache.org/r/10987/#comment43925>

    This file do not have a license. We need to either put a license here or configure RAT
to skip checking this file.



testdata/DatatypeTestData-import-lite.txt
<https://reviews.apache.org/r/10987/#comment43926>

    This file do not have a license. We need to either put a license here or configure RAT
to skip checking this file.


Jarcec

- Jarek Cecho


On May 23, 2013, 5:55 p.m., Shuaishuai Nie wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10987/
> -----------------------------------------------------------
> 
> (Updated May 23, 2013, 5:55 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Description
> -------
> 
> Add the MS Sqoop connector tests that test integration scenarios with SQL Server to the
repo.
> 
> 
> This addresses bug SQOOP-1035.
>     https://issues.apache.org/jira/browse/SQOOP-1035
> 
> 
> Diffs
> -----
> 
>   conf/MSTest.properties PRE-CREATION 
>   src/test/com/cloudera/sqoop/hive/TestHiveImport.java 462ccf1 
>   src/test/com/cloudera/sqoop/manager/SQLServerManagerImportManualTest.java 27860c2 
>   src/test/org/apache/sqoop/manager/MSSQLTestData.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/MSSQLTestDataFileParser.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/MSSQLTestUtils.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/ManagerCompatExport.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerDatatypeExportDelimitedFileManualTest.java
PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerDatatypeExportSequenceFileManualTest.java
PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerDatatypeImportDelimitedFileManualTest.java
PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerDatatypeImportSequenceFileManualTest.java
PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerHiveImportManualTest.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerManagerManualTest.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerMultiColsManualTest.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerMultiMapsManualTest.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerParseMethodsManualTest.java PRE-CREATION

>   src/test/org/apache/sqoop/manager/SQLServerQueryManualTest.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerSplitByManualTest.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/SQLServerWhereManualTest.java PRE-CREATION 
>   testdata/DatatypeTestData-export-lite.txt PRE-CREATION 
>   testdata/DatatypeTestData-import-lite.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/10987/diff/
> 
> 
> Testing
> -------
> 
> All tests passing after applying the patch
> 
> 
> Thanks,
> 
> Shuaishuai Nie
> 
>


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