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: SQOOP-846 Provide Netezza connector
Date Sat, 02 Mar 2013 19:26:26 GMT

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


Hi Venkat,
thank you very much for incorporating all my suggestions. I believe that we're almost there!
I do have just couple of additional high level notes and few comments (mostly just nits):

1) Tests are passing for me - it was something specific to my environment. Thank you for checking
though!

2) The docs are not compiling for me:

     [exec] asciidoc: ERROR: connectors.txt: line 288: [blockdef-listing] missing closing
delimiter

3) I would like to see more tests, especially about testing the various extra arguments. But
I'm open to resolve that in follow up jira. No need to postpone this task even further.


src/docs/user/connectors.txt
<https://reviews.apache.org/r/9543/#comment36773>

    Nit: Copy paste from PostgreSQL :-)



src/docs/user/connectors.txt
<https://reviews.apache.org/r/9543/#comment36776>

    It seems that the start of second column in the header is somewhere else than rest of
the rows. This seems to be causing very weird typesetting of the table. Would you mind taking
a look?



src/docs/user/connectors.txt
<https://reviews.apache.org/r/9543/#comment36777>

    Nit: Would you mind prepending the argument with "\--"



src/docs/user/connectors.txt
<https://reviews.apache.org/r/9543/#comment36778>

    Nit: Would you mind prepending the argument with "\--"



src/docs/user/connectors.txt
<https://reviews.apache.org/r/9543/#comment36779>

    Nit: Would you mind prepending the argument with "\--"



src/docs/user/connectors.txt
<https://reviews.apache.org/r/9543/#comment36771>

    Nit: Please remove the trailing whitespace.



src/docs/user/connectors.txt
<https://reviews.apache.org/r/9543/#comment36774>

    Would you mind enclosing the execution example in "---"? It will be then typeset differently
in the HTML docs.



src/docs/user/connectors.txt
<https://reviews.apache.org/r/9543/#comment36772>

    Nit: Please remove the trailing whitespace.



src/docs/user/connectors.txt
<https://reviews.apache.org/r/9543/#comment36775>

    Would you mind enclosing the execution example in "---"? It will be then typeset differently
in the HTML docs.



src/java/org/apache/sqoop/manager/DirectNetezzaManager.java
<https://reviews.apache.org/r/9543/#comment36780>

    Nit: Can we put the cause into the constructor?



src/java/org/apache/sqoop/manager/DirectNetezzaManager.java
<https://reviews.apache.org/r/9543/#comment36788>

    Nit: Those two variables seems to be unused in the method.



src/java/org/apache/sqoop/manager/DirectNetezzaManager.java
<https://reviews.apache.org/r/9543/#comment36789>

    Nit: Return value of the methods is already char, is there a reason for the explicit retyping?



src/java/org/apache/sqoop/manager/DirectNetezzaManager.java
<https://reviews.apache.org/r/9543/#comment36790>

    Nit: Return value of the methods is already char, is there a reason for the explicit retyping?



src/java/org/apache/sqoop/manager/DirectNetezzaManager.java
<https://reviews.apache.org/r/9543/#comment36783>

    Reading current code, I've realized that that I've misunderstood the purpose last time.
The super.getNetezzaExtraOpts() is adding only NETEZZA_DATASLICE_ALIGNED_ACCESS_OPT, but that
is always set to true in the Direct connector. Thus I believe that we should not get the super
here as we would offer extra argument that is not used in the direct connector. Sorry for
the confusion!



src/java/org/apache/sqoop/manager/NetezzaManager.java
<https://reviews.apache.org/r/9543/#comment36781>

    It seems to me that this method just call the parent one, but that will be done by java
automatically, right?



src/java/org/apache/sqoop/manager/NetezzaManager.java
<https://reviews.apache.org/r/9543/#comment36782>

    Shouldn't be safer operation to rollback instead of commit here? I would expect that the
rest of code will commit any changes that it needed to do prior calling close method.



src/java/org/apache/sqoop/manager/NetezzaManager.java
<https://reviews.apache.org/r/9543/#comment36784>

    I found this quite confusing. The if(cmdLine.hasOption()) do not have else and thus that
path of the execution will leave this property unset. It seems that the default value is false
(per NetezzaDataDrivenDBInputFormat). I would suggest to either set this property in all branches
or depend on the default value.



src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableExportMapper.java
<https://reviews.apache.org/r/9543/#comment36791>

    This variable seems to be unused.



src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableExportMapper.java
<https://reviews.apache.org/r/9543/#comment36792>

    This variables seems to be unused.



src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableExportMapper.java
<https://reviews.apache.org/r/9543/#comment36785>

    I believe that Sqoop default NULL substitution string is "null" (lowercase). It might
be good idea to stay consistent.



src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableImportMapper.java
<https://reviews.apache.org/r/9543/#comment36786>

    I believe that Sqoop default NULL substitution string is "null" (lowercase). It might
be good idea to stay consistent.



src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaJDBCStatementRunner.java
<https://reviews.apache.org/r/9543/#comment36787>

    Can we rename this method to "printException"?


Jarcec

- Jarek Cecho


On Feb. 28, 2013, 8:06 a.m., Venkat Ranganathan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9543/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2013, 8:06 a.m.)
> 
> 
> Review request for Sqoop and Jarek Cecho.
> 
> 
> Description
> -------
> 
> This addresses SQOOP-846 (provide a Netezza connector)
> 
> 
> This addresses bug SQOOP-846.
>     https://issues.apache.org/jira/browse/SQOOP-846
> 
> 
> Diffs
> -----
> 
>   src/docs/user/connectors.txt 7dd2a2e 
>   src/java/org/apache/sqoop/lib/DelimiterSet.java 4e9bcab 
>   src/java/org/apache/sqoop/manager/DefaultManagerFactory.java 54eb258 
>   src/java/org/apache/sqoop/manager/DirectNetezzaManager.java PRE-CREATION 
>   src/java/org/apache/sqoop/manager/MySQLUtils.java ef18818 
>   src/java/org/apache/sqoop/manager/NetezzaManager.java PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaDBDataSliceSplitter.java PRE-CREATION

>   src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableExportMapper.java
PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableImportMapper.java
PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableRecordExportMapper.java
PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableTextExportMapper.java
PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaJDBCStatementRunner.java PRE-CREATION

>   src/java/org/apache/sqoop/mapreduce/netezza/NetezzaDataDrivenDBInputFormat.java PRE-CREATION

>   src/java/org/apache/sqoop/mapreduce/netezza/NetezzaExternalTableExportJob.java PRE-CREATION

>   src/java/org/apache/sqoop/mapreduce/netezza/NetezzaExternalTableImportJob.java PRE-CREATION

>   src/java/org/apache/sqoop/mapreduce/netezza/NetezzaExternalTableInputFormat.java PRE-CREATION

>   src/java/org/apache/sqoop/mapreduce/netezza/NetezzaExternalTableInputSplit.java PRE-CREATION

>   src/test/com/cloudera/sqoop/manager/DirectNetezzaExportManualTest.java PRE-CREATION

>   src/test/com/cloudera/sqoop/manager/NetezzaImportManualTest.java PRE-CREATION 
>   src/test/com/cloudera/sqoop/manager/NetezzaTestUtils.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/9543/diff/
> 
> 
> Testing
> -------
> 
> Ran all sqoop tests.   Ran Netezza manual tests against Netezza VMs version 6 and 7
> 
> 
> Thanks,
> 
> Venkat Ranganathan
> 
>


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