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 13445: Fix for SQOOP-1167
Date Wed, 04 Sep 2013 06:09:45 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message