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


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > 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.

Thanks Jarek for reviewing and feedback.

1)  Great - I could run it with NPS 6 and 7.  And glad that it is now passing for you.   
2)  Fixed the docs.
3)  I agree.   I have couple of manual tests but will integrate them by filing followon JIRAs

Will update new patch after testing and making sure the documentation is in order also

Thanks
Venkat


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > src/docs/user/connectors.txt, line 257
> > <https://reviews.apache.org/r/9543/diff/8/?file=263535#file263535line257>
> >
> >     Nit: Copy paste from PostgreSQL :-)

Thanks :)


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > src/docs/user/connectors.txt, lines 259-276
> > <https://reviews.apache.org/r/9543/diff/8/?file=263535#file263535line259>
> >
> >     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?

Yes.   Fixing it


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > src/docs/user/connectors.txt, line 262
> > <https://reviews.apache.org/r/9543/diff/8/?file=263535#file263535line262>
> >
> >     Nit: Would you mind prepending the argument with "\--"

I based on the Postgresql connector doc which was not prepending the --.  But adding it as
it seems more natural for users to expect


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > src/docs/user/connectors.txt, lines 294-296
> > <https://reviews.apache.org/r/9543/diff/8/?file=263535#file263535line294>
> >
> >     Would you mind enclosing the execution example in "---"? It will be then typeset
differently in the HTML docs.

Yes.  Good thing to do


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, lines 134-135
> > <https://reviews.apache.org/r/9543/diff/8/?file=263538#file263538line134>
> >
> >     Nit: Those two variables seems to be unused in the method.

Fixed.  It was on exportTable.


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, lines 141-142
> > <https://reviews.apache.org/r/9543/diff/8/?file=263538#file263538line141>
> >
> >     Nit: Return value of the methods is already char, is there a reason for the
explicit retyping?

When I moved the check from execution to client side I did not change it where the method
used was typing it to int


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/NetezzaManager.java, line 96
> > <https://reviews.apache.org/r/9543/diff/8/?file=263540#file263540line96>
> >
> >     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.

Yes.  That is true.  It seems that this is a carry over from the PostgresqlManager.   I will
file a JIRA to fix the PostgresqlManager also


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/NetezzaManager.java, line 223
> > <https://reviews.apache.org/r/9543/diff/8/?file=263540#file263540line223>
> >
> >     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.

Sorry about the confusion.  I cleaned it up and removed the else clause with the default value
set unconditionally and optionally reset


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableImportMapper.java,
line 89
> > <https://reviews.apache.org/r/9543/diff/8/?file=263543#file263543line89>
> >
> >     I believe that Sqoop default NULL substitution string is "null" (lowercase).
It might be good idea to stay consistent.

Good point.   Changing it to be consistent


> On March 2, 2013, 7:26 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaJDBCStatementRunner.java,
line 47
> > <https://reviews.apache.org/r/9543/diff/8/?file=263546#file263546line47>
> >
> >     Can we rename this method to "printException"?

Yes.  Makes sense.


- Venkat


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


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