Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Sqoop >> mail # dev >> Review Request: SQOOP-621: Requesting support for upsert export with MySQL


+
Jarek Cecho 2012-10-15, 00:52
+
Cheolsoo Park 2012-10-17, 18:01
Copy link to this message
-
Re: Review Request: SQOOP-621: Requesting support for upsert export with MySQL


> On Oct. 17, 2012, 6:01 p.m., Cheolsoo Park wrote:
> > Thanks for the patch Jarcec. Looks good overall, few comments:
> >
> > 1. Please update the doc.
> > 2. Please fix ant checkstyle errors. I see 10 errors.
> > 3. Few more minor comments below.

Hi sir,
thank you very much for your review. I appreciate your time. I'll upload updated patch shortly.
> On Oct. 17, 2012, 6:01 p.m., Cheolsoo Park wrote:
> > src/java/org/apache/sqoop/manager/MySQLManager.java, line 30
> > <https://reviews.apache.org/r/7588/diff/1/?file=176494#file176494line30>
> >
> >     Minor. Can you please follow the ordering of import statements?

I'm not exactly sure what you mean by "ordering", but I'll try to improve that in my next patch version.
> On Oct. 17, 2012, 6:01 p.m., Cheolsoo Park wrote:
> > src/java/org/apache/sqoop/manager/MySQLManager.java, line 39
> > <https://reviews.apache.org/r/7588/diff/1/?file=176494#file176494line39>
> >
> >     Same.

-^
> On Oct. 17, 2012, 6:01 p.m., Cheolsoo Park wrote:
> > src/test/com/cloudera/sqoop/manager/JdbcMySQLExportTest.java, line 170
> > <https://reviews.apache.org/r/7588/diff/1/?file=176497#file176497line170>
> >
> >     Can you add more test cases?
> >    
> >     Off the top of my head, I can think of at least 3 cases:
> >     1) upsert all new rows.
> >     2) upsert all existing rows.
> >     3) upsert some new rows and some existing rows.
> >    
> >     Your test case combines 1 and 2. Wouldn't it make sense to break it up into separate cases and add a new case for 3?
> >    
> >     Let me know what you think.

Honestly speaking, I've copy&pasted Oracle's upsert test, but you're right. It would make more sense to be conservative and test also the third case.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7588/#review12518
-----------------------------------------------------------
On Oct. 15, 2012, 12:52 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7588/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2012, 12:52 a.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> I've implemented upsert functionality using MySQL clause INSERT INTO ... ON DUPLICATE KEY UPDATE. This clause have slightly different purpose than Oracle's MERGE statement and therefore the functionality is slightly different. I've provided warning message notifying user that column names specified in --update-key parameter are not going to be used.
>
>
> This addresses bug SQOOP-621.
>     https://issues.apache.org/jira/browse/SQOOP-621
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/manager/DirectMySQLManager.java 2e8d63e5b45f0b74b0ccce9e9bff4a1f798bb6a8
>   src/java/org/apache/sqoop/manager/MySQLManager.java a817aa41fee3385b6e8796cadd4c09319b0b6e68
>   src/java/org/apache/sqoop/mapreduce/JdbcUpdateExportJob.java c8e17c236f272387fd14ef6d222cc0edb5fe59ab
>   src/java/org/apache/sqoop/mapreduce/mysql/MySQLUpsertOutputFormat.java PRE-CREATION
>   src/test/com/cloudera/sqoop/manager/JdbcMySQLExportTest.java f00cac4eb7c0600dc567717eff391909a831c6fb
>   src/test/com/cloudera/sqoop/manager/ManualMySQLTests.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/7588/diff/
>
>
> Testing
> -------
>
> I've added new unit tests plus live testing.
>
>
> Thanks,
>
> Jarek Cecho
>
>

+
Jarek Cecho 2012-10-17, 23:33
+
Cheolsoo Park 2012-10-18, 03:27