> On Sept. 3, 2013, 9:18 a.m., Jarek Cecho wrote: > > Hi Venkat, > > thank you very much for working on this JIRA! Couple of notes (mostly nits):
Thanks Jarcec for a quick review. Will update the patch after addressing the comments. > On Sept. 3, 2013, 9:18 a.m., Jarek Cecho wrote: > > src/docs/user/hcatalog.txt, lines 112-115 > > <https://reviews.apache.org/r/13445/diff/1/?file=339243#file339243line112> > > > > Can we made a warning box here that not all direct connectors are > > supporting the HCatalog integration out of the box? Good idea. Will do > On Sept. 3, 2013, 9:18 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableImportMapper.java, > > lines 197-200 > > <https://reviews.apache.org/r/13445/diff/1/?file=339251#file339251line197> > > > > Since this class has been changed to abstract, would it make sense to > > define this method as abstract as well? Yes, makes sense. Will do. > On Sept. 3, 2013, 9:18 a.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableTextImportMapper.java, > > lines 32-33 > > <https://reviews.apache.org/r/13445/diff/1/?file=339252#file339252line32> > > > > Nit: These variables seems to be unused. Let me remove them. > On Sept. 3, 2013, 9:18 a.m., Jarek Cecho wrote: > > src/test/org/apache/sqoop/hcat/HCatalogTestUtils.java, line 603 > > <https://reviews.apache.org/r/13445/diff/1/?file=339268#file339268line603> > > > > 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? I will double check and cleanup. > On Sept. 3, 2013, 9:18 a.m., Jarek Cecho wrote: > > src/test/org/apache/sqoop/manager/netezza/DirectNetezzaHCatExportManualTest.java, > > line 163 > > <https://reviews.apache.org/r/13445/diff/1/?file=339271#file339271line163> > > > > Super nit: s/testStaic/testStatic/ Thanks for catching it! > On Sept. 3, 2013, 9:18 a.m., Jarek Cecho wrote: > > src/test/org/apache/sqoop/manager/netezza/DirectNetezzaHCatImportManualTest.java, > > line 167 > > <https://reviews.apache.org/r/13445/diff/1/?file=339272#file339272line167> > > > > Super nit: s/testStaic/testStatic/ Thanks - will fix > On Sept. 3, 2013, 9:18 a.m., Jarek Cecho wrote: > > src/test/org/apache/sqoop/manager/netezza/NetezzaExportManualTest.java, > > lines 177-180 > > <https://reviews.apache.org/r/13445/diff/1/?file=339273#file339273line177> > > > > I'm wondering if this method "synonym" is necessary when it just call > > different protected method? I think one of them creates a file and another a hcatalog table as source. Let me double check > On Sept. 3, 2013, 9:18 a.m., Jarek Cecho wrote: > > src/test/org/apache/sqoop/manager/netezza/NetezzaTestUtils.java, line 34 > > <https://reviews.apache.org/r/13445/diff/1/?file=339275#file339275line34> > > > > 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? I think it is good to add them. I will check what I have in my test env and add anything else if needed. Thanks for the suggestion - Venkat ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13445/#review25846 ----------------------------------------------------------- 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 > >
