Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Sqoop, mail # dev - Review Request: SQOOP-846 Provide Netezza connector


Copy link to this message
-
Re: Review Request: SQOOP-846 Provide Netezza connector
Jarek Cecho 2013-03-02, 19:26

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