> 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
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/DirectNetezzaManager.java, lines 234-236
> > <https://reviews.apache.org/r/9543/diff/5/?file=261951#file261951line234>
> >
> > 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 Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/NetezzaManager.java, line 208
> > <https://reviews.apache.org/r/9543/diff/5/?file=261953#file261953line208>
> >
> > I'm not quite sure what is the reasoning for skipping extra parameter
> > checking when there is only one mapper?
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
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/manager/NetezzaManager.java, lines 209-213
> > <https://reviews.apache.org/r/9543/diff/5/?file=261953#file261953line209>
> >
> > Can we put this into standalone method getNetezzaExtraOpts() similarly
> > as we've done in NetezzaDirectManager?
Yes
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableExportJob.java,
> > line 39
> > <https://reviews.apache.org/r/9543/diff/5/?file=261956#file261956line39>
> >
> > That comment seems little bit off :-)
I used the mysql file as the starting point :)
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableExportMapper.java,
> > line 44
> > <https://reviews.apache.org/r/9543/diff/5/?file=261957#file261957line44>
> >
> > Shouldn't this be abstract class? It seems to me that it do not make
> > sense when it would be used directly.
Yes good point.
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableExportMapper.java,
> > line 45
> > <https://reviews.apache.org/r/9543/diff/5/?file=261957#file261957line45>
> >
> > Can we inherit from SqoopMapper?
Yes, that provides the ability to handle verbose setting also
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableExportMapper.java,
> > line 101
> > <https://reviews.apache.org/r/9543/diff/5/?file=261957#file261957line101>
> >
> > Can we make similar check on the Sqoop client side? I'm afraid that
> > this warning will get lost as it's printed in the mapper task and I'm not
> > expecting users to read it up there.
Great point. I updated this for both direct mode import and export table so
that exception is printed early.
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableInputSplitter.java,
> > line 30
> > <https://reviews.apache.org/r/9543/diff/5/?file=261961#file261961line30>
> >
> > I would suggest to rename this into NetezzaExternalTableInputSplit
> > instead of "..Splitter".
Done
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaJDBCStatementRunner.java,
> > line 64
> > <https://reviews.apache.org/r/9543/diff/5/?file=261964#file261964line64>
> >
> > This chaining of exceptions seems problematic to me. The initCause()
> > might not be allowed to call as the exception cause might be already
> > initialized.
> >
> > It seems that there might be maximally two exceptions. If that is
> > correct then I suggest to return only one instead of the chain. I would
> > personally prioritized the one catched on line 88 over the one on line 95.
Fixed. Good catch
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/test/com/cloudera/sqoop/manager/DirectNetezzaExportManualTest.java,
> > line 233
> > <https://reviews.apache.org/r/9543/diff/5/?file=261965#file261965line233>
> >
> > I would vote for not doing this. The file:// should be default value
> > and by explicitly specifying this, we will loose the ability to run the
> > tests against real Hadoop cluster.
Good point - I removed it
> On Feb. 27, 2013, 1:01 a.m., Jarek Cecho wrote:
> > src/test/com/cloudera/sqoop/manager/NetezzaTestUtils.java, lines 72-73
> > <https://reviews.apache.org/r/9543/diff/5/?file=261967#file261967line72>
> >
> > Can we print out used Netezza URL, something like:
> >
> > LOG.info("Using Netezza URL: " + url.toString());
> >
> > My motivation here is to help users running the test to see what
> > exactly is Sqoop trying to do. It helped me, so I can imagine that it might
> > help others as well.
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:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9543/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2013, 1:53 a.m.)
>
>
> Review request for Sqoop and Jarek Cecho.
>
>
> Description
> -------
>
> This addresses SQOOP-846 (provide a Netezza connector)
>
>
> This addresses bug SQOOP-846.
> https://issues.apache.org/jira/browse/SQOOP-846
>
>
> Diffs
> -----
>
> src/docs/user/connectors.txt 7dd2a2e
> src/java/org/apache/sqoop/lib/DelimiterSet.java 4e9bcab
> src/java/org/apache/sqoop/manager/DefaultManagerFactory.java 54eb258
> src/java/org/apache/sqoop/manager/DirectNetezzaManager.java PRE-CREATION
> src/java/org/apache/sqoop/manager/MySQLUtils.java ef18818
> src/java/org/apache/sqoop/manager/NetezzaManager.java PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaDBDataSliceSplitter.java
> PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableExportMapper.java
> PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableImportMapper.java
> PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableRecordExportMapper.java
> PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaExternalTableTextExportMapper.java
> PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/db/netezza/NetezzaJDBCStatementRunner.java
> PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/netezza/NetezzaDataDrivenDBInputFormat.java
> PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/netezza/NetezzaExternalTableExportJob.java
> PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/netezza/NetezzaExternalTableImportJob.java
> PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/netezza/NetezzaExternalTableInputFormat.java
> PRE-CREATION
>
> src/java/org/apache/sqoop/mapreduce/netezza/NetezzaExternalTableInputSplit.java
> PRE-CREATION
> src/test/com/cloudera/sqoop/manager/DirectMySQLExportTest.java eac7836
> src/test/com/cloudera/sqoop/manager/DirectNetezzaExportManualTest.java
> PRE-CREATION
> src/test/com/cloudera/sqoop/manager/NetezzaImportManualTest.java
> PRE-CREATION
> src/test/com/cloudera/sqoop/manager/NetezzaTestUtils.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/9543/diff/
>
>
> Testing
> -------
>
> Ran all sqoop tests. Ran Netezza manual tests against Netezza VMs version 6
> and 7
>
>
> Thanks,
>
> Venkat Ranganathan
>
>
