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
Hi Jarcec
I have to spin one more patch    I inadvertently added DirectMySQLTest when I was preparing this patch.   Will respin without that file.  In the meanwhile if you have review comments on all the other files, please continue with the review.   The only new change that I am planning to do is to remove this file

Venkat

From: Venkat Ranganathan
Sent: Wednesday, February 27, 2013 9:04 PM
To: Jarek Cecho
Cc: Venkat Ranganathan ; Sqoop
Subject: Re: Review Request: SQOOP-846 Provide Netezza connector

      This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9543/  
  On February 27th, 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 February 27th, 2013, 1:01 a.m., Jarek Cecho wrote:

          src/java/org/apache/sqoop/manager/DirectNetezzaManager.java (Diff revision 5)  
None
          34 import org.apache.sqoop.mapreduce.db.netezza.NetezzaDataDrivenDBInputFormat;
          35 import org.apache.sqoop.mapreduce.db.netezza.NetezzaExternalTableExportJob;
          36 import org.apache.sqoop.mapreduce.db.netezza.NetezzaExternalTableImportJob;

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.sqlserverSure - would do

  On February 27th, 2013, 1:01 a.m., Jarek Cecho wrote:

          src/java/org/apache/sqoop/manager/DirectNetezzaManager.java (Diff revision 5)  
None
          131     try {
          132       handleNetezzaExtraArgs(options);
          133     } catch (SqoopOptions.InvalidOptionsException ioe) {
          134       throw (ExportException) new ExportException(ioe.getMessage())
          135           .initCause(ioe);
          136     }

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 February 27th, 2013, 1:01 a.m., Jarek Cecho wrote:

          src/java/org/apache/sqoop/manager/DirectNetezzaManager.java (Diff revision 5)  
None
          211     if (extraArgs != null && extraArgs.length == 0

Shouldn't this condition be something like:

...extraArgs.length != 0 ...Yes!

  On February 27th, 2013, 1:01 a.m., Jarek Cecho wrote:

          src/java/org/apache/sqoop/manager/DirectNetezzaManager.java (Diff revision 5)  
None
          212         && ConfigurationHelper.getConfNumMaps(conf) > 1) {

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 February 27th, 2013, 1:01 a.m., Jarek Cecho wrote:

          src/java/org/apache/sqoop/manager/DirectNetezzaManager.java (Diff revision 5)  
None
          229         SqoopOptions.InvalidOptionsException ioe = new SqoopOptions.InvalidOptionsException(
          230             pe.getMessage());
          231         ioe.initCause(pe);
          232         throw ioe;

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

  On February 27th, 2013, 1:01 a.m., Jarek Cecho wrote:

          src/java/org/apache/sqoop/manager/DirectNetezzaManager.java (Diff revision 5)  
None
          234     } else {
          235       conf.setBoolean(NETEZZA_DATASLICE_ALIGNED_ACCESS_OPT, true);
          236     }

It seems to me that this property is set in both branches of the if-else statement. Would it make sense to set it at one place then?Yes, good point

  On February 27th, 2013, 1:01 a.m., Jarek Cecho wrote:

          src/java/org/apache/sqoop/manager/NetezzaManager.java (Diff revision 5)  
None {'text': '  private void handleNetezzaImportExtraArgs(ImportJobContext context)', 'line': 199}
          208         && ConfigurationHelper.getConfNumMaps(conf) > 1) {

I'm not quite sure what is the reasoning for skipping extra parameter checking when there i