Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Sqoop, mail # dev - Review Request 13445: Fix for SQOOP-1167


Copy link to this message
-
Re: Review Request 13445: Fix for SQOOP-1167
Venkat Ranganathan 2013-09-04, 06:09


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

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: