----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13445/#review25846 -----------------------------------------------------------
Hi Venkat, thank you very much for working on this JIRA! Couple of notes (mostly nits): src/docs/user/hcatalog.txt <https://reviews.apache.org/r/13445/#comment50400> Can we made a warning box here that not all direct connectors are supporting the HCatalog integration out of the box? src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableImportMapper.java <https://reviews.apache.org/r/13445/#comment50401> Since this class has been changed to abstract, would it make sense to define this method as abstract as well? src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableTextImportMapper.java <https://reviews.apache.org/r/13445/#comment50405> Nit: These variables seems to be unused. src/test/org/apache/sqoop/hcat/HCatalogTestUtils.java <https://reviews.apache.org/r/13445/#comment50402> The "IF EXISTS" clause seems to be failing on Netezza 6. I currently do not have access to version 7, but I'm wondering if this is new feature of version 7? Also it seems that couple of other classes have their own "DROP TABLE" statement definition. Maybe it would be worth cleaning that out and using only this util class to generate the drop table statement? src/test/org/apache/sqoop/manager/netezza/DirectNetezzaHCatExportManualTest.java <https://reviews.apache.org/r/13445/#comment50403> Super nit: s/testStaic/testStatic/ src/test/org/apache/sqoop/manager/netezza/DirectNetezzaHCatImportManualTest.java <https://reviews.apache.org/r/13445/#comment50404> Super nit: s/testStaic/testStatic/ src/test/org/apache/sqoop/manager/netezza/NetezzaExportManualTest.java <https://reviews.apache.org/r/13445/#comment50406> I'm wondering if this method "synonym" is necessary when it just call different protected method? src/test/org/apache/sqoop/manager/netezza/NetezzaTestUtils.java <https://reviews.apache.org/r/13445/#comment50407> I had difficulties to use system properties defined in this file to override connection credentials. I was trying to set clonevm of junit ant task to true, but that did not help me, so I've ended up tweaking the build system with following patch: diff --git a/build.xml b/build.xml index 39c6337..b56bc55 100644 --- a/build.xml +++ b/build.xml @@ -322,6 +322,12 @@ <property name="sqoop.test.db2.connectstring.username" value="SQOOP" /> <property name="sqoop.test.db2.connectstring.password" value="SQOOP" /> + <property name="sqoop.test.netezza.host" value="nz-host" /> + <property name="sqoop.test.netezza.port" value="5480" /> + <property name="sqoop.test.netezza.username" value="ADMIN" /> + <property name="sqoop.test.netezza.password" value="password" /> + <property name="sqoop.test.netezza.db.name" value="SQOOP" /> + <property name="sqoop.test.netezza.table.name" value="EMPNZ" /> <condition property="windows"> <os family="windows" /> @@ -901,6 +907,13 @@ <sysproperty key="sqoop.test.db2.connectstring.username" value="${sqoop.test.db2.connectstring.username}" /> <sysproperty key="sqoop.test.db2.connectstring.password" value="${sqoop.test.db2.connectstring.password}" /> + <sysproperty key="sqoop.test.netezza.host" value="${sqoop.test.netezza.host}" /> + <sysproperty key="sqoop.test.netezza.port" value="${sqoop.test.netezza.port}" /> + <sysproperty key="sqoop.test.netezza.username" value="${sqoop.test.netezza.username}" /> + <sysproperty key="sqoop.test.netezza.password" value="${sqoop.test.netezza.password}" /> + <sysproperty key="sqoop.test.netezza.db.name" value="${sqoop.test.netezza.db.name}" /> + <sysproperty key="sqoop.test.netezza.table.name" value="${sqoop.test.netezza.table.name}" /> + <!-- Location of Hive logs --> <!--<sysproperty key="hive.log.dir" value="${test.build.data}/sqoop/logs"/> --> Do you think that we should include something like this in the patch? Jarcec - Jarek Cecho On Aug. 9, 2013, 6:11 p.m., Venkat Ranganathan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13445/ > ----------------------------------------------------------- > > (Updated Aug. 9, 2013, 6:11 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1167 > https://issues.apache.org/jira/browse/SQOOP-1167 > > > Repository: sqoop-trunk > > > Description > ------- > > This patch implements the following > > Enhance HCat support to allow direct mode connectors by > Creating helpers for export and import hcat mappers and refactor hcat > mappers to use that > Adding additional method for connection managers to declare their ability > to exploit this feature > Make the detection of compatibility between hcat and direct mode managers > after connection managers are created > As an example usecase, fix the netezza implementation to > Abstract the Netezza direct mode mappers and add hcat support > Fix Netezza connector implementation issues etc > Add documentation > Add Netezza tests to third party test suite > Move Netezza tests to org.apache.namespace to be consistent with > requirements for newly added tests > > > Diffs > ----- > > src/docs/user/hcatalog.txt b8e495e > src/java/org/apache/sqoop/manager/ConnManager.java f4b22f9 > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java 4f36bf6 > src/java/org/apache/sqoop/mapreduce/ExportJobBase.java d0be570 > src/java/org/apache/sqoop/mapreduce/ImportJobBase.java ab7f21e > > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableExportMapper.java > 22b7af5 > > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableHCatExportMapper.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableHCatImportMapper.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableImportMapper.java > bcdc9e1 > > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableTextImportMapper.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaJDBCStatementRunner.java > 3a5df40 > src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatExportHelper.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatExportMapper.java 539cedf > src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportHelper.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatImportMapper.java 4f0ff1b > > src/java/org/apache/sqoop/mapreduce/netezza/NetezzaExternalTableExportJob.java > 7caf9be > > src/java/org/apache/sqoop/mapreduce/netezza/NetezzaExternalTableImportJob.java > 0f7c1b0 > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 0eca991 > src/test/com/cloudera/sqoop/ThirdPartyTests.java 0a080b6 > src/test/com/cloudera/sqoop/manager/DirectNetezzaExportManualTest.java > aace924 > src/test/com/cloudera/sqoop/manager/NetezzaExportManualTest.java 43dd254 > src/test/com/cloudera/sqoop/manager/NetezzaImportManualTest.java 86f5bdd > src/test/com/cloudera/sqoop/manager/NetezzaTestUtils.java 4bf05b8 > src/test/org/apache/sqoop/hcat/HCatalogExportTest.java 77bafcc > src/test/org/apache/sqoop/hcat/HCatalogImportTest.java 293015e > src/test/org/apache/sqoop/hcat/HCatalogTestUtils.java ddae5f5 > src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java da803d0 > > src/test/org/apache/sqoop/manager/netezza/DirectNetezzaExportManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/netezza/DirectNetezzaHCatExportManualTest.java > PRE-CREATION > > src/test/org/apache/sqoop/manager/netezza/DirectNetezzaHCatImportManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/netezza/NetezzaExportManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/netezza/NetezzaImportManualTest.java > PRE-CREATION > src/test/org/apache/sqoop/manager/netezza/NetezzaTestUtils.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/13445/diff/ > > > Testing > ------- > > Add additional tests for testing this feature. Ran all tests and all of > them passed > > > Thanks, > > Venkat Ranganathan > >
