----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9543/#review17010 -----------------------------------------------------------
Hi Venkat, thank you very much for putting Netezza connector together. I greatly appreciate your time and effort. I've noticed that updated patch number 2 have only 12kb and is not the same as was uploaded into review board. Would you mind uploading the latest patch to the review board? It's much more easier for reviewer (well, it's much more easier for me). Nevertheless, I've done high level review: 1) Would you mind removing trailing white space characters? 2) Would you mind adding appropriate section(s) into user guide? src/java/org/apache/sqoop/manager/DirectNetezzaManager.java <https://reviews.apache.org/r/9543/#comment36052> I felt that org.apache.sqoop.mapreduce packages is becoming a big mess, so I've started adding new connector-specific code into their own packages. For example: org.apache.sqoop.mapreduce.mysql org.apache.sqoop.mapreduce.sqlserver I would suggest to put the mapreduce files for Netezza into their own respective package: org.apache.sqoop.mapreduce.netezza What do you think? src/java/org/apache/sqoop/manager/DirectNetezzaManager.java <https://reviews.apache.org/r/9543/#comment36059> Tests are suggesting that staging table in Direct mode is not supported, though this class do not override supportsStagingForExport method that is returning true in parent class. src/java/org/apache/sqoop/manager/DirectNetezzaManager.java <https://reviews.apache.org/r/9543/#comment36053> Would you mind introducing finally block and closing the PreparedStatement and ResultSet? Something like } finally { if(rc != null) { rc.close(); } if(ps != null) { ps.close(); } } src/java/org/apache/sqoop/manager/DirectNetezzaManager.java <https://reviews.apache.org/r/9543/#comment36054> Nit: Would you mind putting the catch keyword on the same line as closing "}". The rest of code is following this code style, so it would be great to remain consistent. src/java/org/apache/sqoop/manager/DirectNetezzaManager.java <https://reviews.apache.org/r/9543/#comment36055> I can see that you're parsing extra arguments on several places (exportTable, importTable). What about moving this code into constructor? src/java/org/apache/sqoop/manager/MySQLUtils.java <https://reviews.apache.org/r/9543/#comment36056> Nit: Trailing white space characters. src/java/org/apache/sqoop/manager/MySQLUtils.java <https://reviews.apache.org/r/9543/#comment36057> Nit: Trailing white space characters. src/test/com/cloudera/sqoop/manager/NetezzaTestUtils.java <https://reviews.apache.org/r/9543/#comment36058> I'm not sure that you want to have your private IP publicly available in the source code. Jarcec - Jarek Cecho On Feb. 22, 2013, 7:58 a.m., Venkat Ranganathan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9543/ > ----------------------------------------------------------- > > (Updated Feb. 22, 2013, 7:58 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/java/org/apache/sqoop/manager/DirectNetezzaManager.java PRE-CREATION > src/java/org/apache/sqoop/manager/MySQLUtils.java ef18818 > src/java/org/apache/sqoop/mapreduce/db/NetezzaExternalTableExportJob.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/db/NetezzaExternalTableExportMapper.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/db/NetezzaExternalTableImportMapper.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 > >
