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

Switch to Threaded View
Sqoop, mail # dev - Review Request: SQOOP-846 Provide Netezza connector


Copy link to this message
-
Re: Review Request: SQOOP-846 Provide Netezza connector
Venkat Ranganathan 2013-02-25, 19:00


> On Feb. 24, 2013, 10:16 p.m., Jarek Cecho wrote:
> > 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?

Thanks Jarcec for reviewing the changes.     I removed the trailing blanks with additional changes and updated a second patch.   Unfortunately, I was not able to upload a new patch as the review board was claiming that some files were not available (because I added a few new files).   Hence I created a patch of only differences from patch 1.   I will try again.   I understand it is difficult to review like this and sorry about that.   Do you have suggestions on I might have done wrong in updating a revision 2?

Yes, I will be adding a section to the User guide (tracked by a separate JIRA) after the review so that I can be clear about options etc.
> On Feb. 24, 2013, 10:16 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, lines 34-36
> > <https://reviews.apache.org/r/9543/diff/1/?file=260675#file260675line34>
> >
> >     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?
> >

Sure,  I moved all Netezza change to mapreduce.db.   But mapreduce.db.netezza is also good.  Since there were not any db specific packages currently, I was not doing that.  But will move the netezza changes  to netezza package.
> On Feb. 24, 2013, 10:16 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, lines 94-99
> > <https://reviews.apache.org/r/9543/diff/1/?file=260675#file260675line94>
> >
> >     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();
> >       }
> >     }

Thanks for catching this!
> On Feb. 24, 2013, 10:16 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, lines 126-128
> > <https://reviews.apache.org/r/9543/diff/1/?file=260675#file260675line126>
> >
> >     I can see that you're parsing extra arguments on several places (exportTable, importTable). What about moving this code into constructor?

Good observation, but if we have an invalid option, do you suggest throwing IllegalArgumentException from the constructor, as the constructor does not throw an exception.  
> On Feb. 24, 2013, 10:16 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/MySQLUtils.java, line 61
> > <https://reviews.apache.org/r/9543/diff/1/?file=260676#file260676line61>
> >
> >     Nit: Trailing white space characters.

Fixed in second revision.  But will create a new combo patch
> On Feb. 24, 2013, 10:16 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/MySQLUtils.java, line 72
> > <https://reviews.apache.org/r/9543/diff/1/?file=260676#file260676line72>
> >
> >     Nit: Trailing white space characters.

Fixed in second revision.  But will create a new combo patch
> On Feb. 24, 2013, 10:16 p.m., Jarek Cecho wrote:
> > src/test/com/cloudera/sqoop/manager/NetezzaTestUtils.java, line 17
> > <https://reviews.apache.org/r/9543/diff/1/?file=260691#file260691line17>

Fixed in second revision.  But will create a new combo patch
- Venkat
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9543/#review17010
On Feb. 22, 2013, 7:58 a.m., Venkat Ranganathan wrote: