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


> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > Hi Venkat,
> > thank you very much for working on this. I really appreciate your time and effort. I've started diving into your patch. I'm not quite done yet, but let me share my thoughts to unblock you.
> >
> > 1) Would you mind running "ant checkstyle" and cleaning up the errors? Some of the errors are already present in trunk, so please feel free to leave them out. Just it would be great if would not include any new errors.
> >
> > 2) Tests do not seem passing for me against Netezza Simulator 6.0. I'm not yet sure why, so I'll investigate that on my end.
> >
> > 3) It seems that you prefer creating chained exceptions in form NewException("String").initCause(oldException). The rest of the code seems to be following different approach and is creating exceptions in form NewException("String", oldException). Do you feel comfortable with change that to have unified code style?
> >

Thanks for the review Jarek.  
1) Ant checkstyle - would do.  Good point.   I missed doing that the last time around
2) Let me know if you find something - it runs fine with my NPS simulators.  
3) Old habit working on WLS Server core in the pre 1.4 days :)   Yes, will switch to the other style

Will update another diff after running tests with new changes
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, lines 34-36
> > <https://reviews.apache.org/r/9543/diff/5/?file=261951#file261951line34>
> >
> >     It seems that I've confused you, so please accept my apologies for that. Would you mind putting the mapreduce specific classes into package "org.apache.sqoop.mapreduce.netezza"? (e.g leaving out the "db" package). You can notice that there are already packages for MySQL and Microsoft SQL Server:
> >    
> >     org.apache.sqoop.mapreduce.mysql
> >     org.apache.sqoop.mapreduce.sqlserver

Sure - would do
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, lines 131-136
> > <https://reviews.apache.org/r/9543/diff/5/?file=261951#file261951line131>
> >
> >     I can see similar code fragment in both exportTable() and importTable() methods (just returning different exception class). I would suggest to put it into constructor instead. The other connectors seems to be doing that already.

I am not a big fan of doing this and making the parsing exception as  a child ( sometimes chained exceptions are eaten while logging etc).  But for consistency will change it
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, line 211
> > <https://reviews.apache.org/r/9543/diff/5/?file=261951#file261951line211>
> >
> >     Shouldn't this condition be something like:
> >    
> >     ...extraArgs.length != 0 ...

Yes!
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, line 212
> > <https://reviews.apache.org/r/9543/diff/5/?file=261951#file261951line212>
> >
> >     I'm not quite sure what is the reasoning for skipping extra parameter checking when there is only one mapper?

For DirectNetezzaManager, it does not make sense to restrict as the log dir and max errors also apply to one mapper case.   For NetezzaManager, with one mapper, there is no point in having a dataslice aligned import, so that check was there.   Removing it for DirectNetezzaManager
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, lines 229-232
> > <https://reviews.apache.org/r/9543/diff/5/?file=261951#file261951line229>
> >
> >     Would you mind simplifying this by doing something like:
> >    
> >     throw new SqoopOptions.InvalidOptionsException(pe.getMessage(), pe);

SqoopOptions does not provide a constructor to initialize the cause (only messages).  I will ignore the cause and just provide the exception msg to the constructor

Yes, good point

I am not sure about this line.   For updates the number of mappers more than 1 causes inconsistencies.   I added a link to the doc but did not reference it specifically.   Will update the doc.   Essentially Netezza does an delete and insert for updates and because of the architecture, updates from multiple mappers to the same table can cause issues

Yes

I used the mysql file as the starting point :)

Yes good point.  

Yes, that provides the ability to handle verbose setting also

Great point.   I updated this for both direct mode import and export table so that exception is printed early.

Done

Fixed.  Good catch

Good point - I removed it

Good point
- Venkat
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9543/#review17076
On Feb. 28, 2013, 1:53 a.m., Venkat Ranganathan wrote: