----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1439/#review1369 -----------------------------------------------------------
Thanks for the patch, overall it looks very good. I have a few comments below. Also, it will be great if you could run checkstyle and findbus and address any generated warnings. /src/java/com/cloudera/sqoop/mapreduce/AvroExportMapper.java <https://reviews.apache.org/r/1439/#comment3143> I think this should result in an exception as it is indicative of data corruption. /src/java/com/cloudera/sqoop/mapreduce/JdbcExportJob.java <https://reviews.apache.org/r/1439/#comment3146> Any reason why this is not refactored inplace in the base class? /src/java/com/cloudera/sqoop/orm/ClassWriter.java <https://reviews.apache.org/r/1439/#comment3145> I understand this is a convenient reuse of logic. However, it will be preferable if the corresponding method on ConnManager class can be overloaded instead. Your thoughts on doing it there? - Arvind On 2011-08-09 21:58:00, Tom White wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1439/ > ----------------------------------------------------------- > > (Updated 2011-08-09 21:58:00) > > > Review request for Sqoop. > > > Summary > ------- > > See https://issues.apache.org/jira/browse/SQOOP-305 > > > This addresses bug SQOOP-305. > https://issues.apache.org/jira/browse/SQOOP-305 > > > Diffs > ----- > > /src/java/com/cloudera/sqoop/mapreduce/AvroExportMapper.java PRE-CREATION > /src/java/com/cloudera/sqoop/mapreduce/AvroImportMapper.java 1154359 > /src/java/com/cloudera/sqoop/mapreduce/AvroInputFormat.java PRE-CREATION > /src/java/com/cloudera/sqoop/mapreduce/AvroRecordReader.java PRE-CREATION > /src/java/com/cloudera/sqoop/mapreduce/ExportJobBase.java 1154359 > /src/java/com/cloudera/sqoop/mapreduce/JdbcExportJob.java 1154359 > /src/java/com/cloudera/sqoop/orm/ClassWriter.java 1154359 > /src/test/com/cloudera/sqoop/TestAvroExport.java PRE-CREATION > /src/test/com/cloudera/sqoop/TestAvroImportExportRoundtrip.java > PRE-CREATION > /src/test/com/cloudera/sqoop/TestExport.java 1154359 > /src/test/com/cloudera/sqoop/TestExportUpdate.java 1154359 > /src/test/com/cloudera/sqoop/testutil/BaseSqoopTestCase.java 1154359 > /src/test/com/cloudera/sqoop/testutil/ExportJobTestCase.java 1154359 > > Diff: https://reviews.apache.org/r/1439/diff > > > Testing > ------- > > > Thanks, > > Tom > >
