You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Tom White <to...@apache.org> on 2011/08/09 23:58:00 UTC
Review Request: Support export from Avro Data Files
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1439/
-----------------------------------------------------------
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
Re: Review Request: Support export from Avro Data Files
Posted by Tom White <to...@apache.org>.
> On 2011-08-09 23:48:42, Arvind Prabhakar wrote:
> > /src/java/com/cloudera/sqoop/mapreduce/JdbcExportJob.java, lines 67-69
> > <https://reviews.apache.org/r/1439/diff/1/?file=31810#file31810line67>
> >
> > Any reason why this is not refactored inplace in the base class?
Because not all subclasses support Avro (JdbCUpdateExportJob and MySQLExportJob). These can be handled in follow up JIRAs.
- Tom
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1439/#review1369
-----------------------------------------------------------
On 2011-08-10 22:51:19, Tom White wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1439/
> -----------------------------------------------------------
>
> (Updated 2011-08-10 22:51:19)
>
>
> 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/manager/ConnManager.java 1156386
> /src/java/com/cloudera/sqoop/mapreduce/AvroExportMapper.java PRE-CREATION
> /src/java/com/cloudera/sqoop/mapreduce/AvroImportMapper.java 1156386
> /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 1156386
> /src/java/com/cloudera/sqoop/mapreduce/JdbcExportJob.java 1156386
> /src/java/com/cloudera/sqoop/orm/ClassWriter.java 1156386
> /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 1156386
> /src/test/com/cloudera/sqoop/TestExportUpdate.java 1156386
> /src/test/com/cloudera/sqoop/testutil/BaseSqoopTestCase.java 1156386
> /src/test/com/cloudera/sqoop/testutil/ExportJobTestCase.java 1156386
>
> Diff: https://reviews.apache.org/r/1439/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Tom
>
>
Re: Review Request: Support export from Avro Data Files
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Support export from Avro Data Files
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1439/#review1399
-----------------------------------------------------------
Ship it!
+1
Please attach the patch to the Jira.
- Arvind
On 2011-08-10 22:51:19, Tom White wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1439/
> -----------------------------------------------------------
>
> (Updated 2011-08-10 22:51:19)
>
>
> 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/manager/ConnManager.java 1156386
> /src/java/com/cloudera/sqoop/mapreduce/AvroExportMapper.java PRE-CREATION
> /src/java/com/cloudera/sqoop/mapreduce/AvroImportMapper.java 1156386
> /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 1156386
> /src/java/com/cloudera/sqoop/mapreduce/JdbcExportJob.java 1156386
> /src/java/com/cloudera/sqoop/orm/ClassWriter.java 1156386
> /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 1156386
> /src/test/com/cloudera/sqoop/TestExportUpdate.java 1156386
> /src/test/com/cloudera/sqoop/testutil/BaseSqoopTestCase.java 1156386
> /src/test/com/cloudera/sqoop/testutil/ExportJobTestCase.java 1156386
>
> Diff: https://reviews.apache.org/r/1439/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Tom
>
>
Re: Review Request: Support export from Avro Data Files
Posted by Tom White <to...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1439/
-----------------------------------------------------------
(Updated 2011-08-10 22:51:19.745107)
Review request for Sqoop.
Changes
-------
Thanks for the review, Arvind. Here's a new patch which addresses your comments.
Summary
-------
See https://issues.apache.org/jira/browse/SQOOP-305
This addresses bug SQOOP-305.
https://issues.apache.org/jira/browse/SQOOP-305
Diffs (updated)
-----
/src/java/com/cloudera/sqoop/manager/ConnManager.java 1156386
/src/java/com/cloudera/sqoop/mapreduce/AvroExportMapper.java PRE-CREATION
/src/java/com/cloudera/sqoop/mapreduce/AvroImportMapper.java 1156386
/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 1156386
/src/java/com/cloudera/sqoop/mapreduce/JdbcExportJob.java 1156386
/src/java/com/cloudera/sqoop/orm/ClassWriter.java 1156386
/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 1156386
/src/test/com/cloudera/sqoop/TestExportUpdate.java 1156386
/src/test/com/cloudera/sqoop/testutil/BaseSqoopTestCase.java 1156386
/src/test/com/cloudera/sqoop/testutil/ExportJobTestCase.java 1156386
Diff: https://reviews.apache.org/r/1439/diff
Testing
-------
Thanks,
Tom