sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Venkat Ranganathan" <n....@live.com>
Subject Re: Review Request: Review request for SQOOP-1035 "Add MS Sqoop Connecter tests to repo"
Date Sun, 12 May 2013 15:48:42 GMT

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


Patch looks good except for a few nits.  It is a big patch and adds lots of tests which is
badly needed.    There are few places where the editor generated TODO comments have to be
removed and the exception handling can be a little bit more precise (instead of printing the
stack trace and ignoring the exception), the exceptions can be rethrown after printing the
stackTrace for debugging purposes if debug mode is enabled (you can check the logger)

Similar issues on test setup.  If the setup throws an exception during one of the methods
(say populate the table), it does not handle it -  rethrow to abort the test.  Similarly getConnection
call ignores exception

Thanks

Venkat


src/test/com/cloudera/sqoop/hive/TestHiveImport.java
<https://reviews.apache.org/r/10987/#comment42139>

    Nit - spaces



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

    Can you please remove the comment - it has been implemented



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

    please remove this



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

    Please remove this.   Don't we want to return null in this case?



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

    Please remove this exception block.  As the method is throwing the SQLException, it should
be OK and the caller can handle the exception



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

    Please remove this block or rethrow the exception



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

    Same as before.   There are too many of these in the files, can you please address all
of them. 


- Venkat Ranganathan


On May 8, 2013, 12:30 a.m., Shuaishuai Nie wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10987/
> -----------------------------------------------------------
> 
> (Updated May 8, 2013, 12:30 a.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