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