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