-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7588/
-----------------------------------------------------------

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7588/#review12518
-----------------------------------------------------------
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.
src/java/org/apache/sqoop/manager/MySQLManager.java
<https://reviews.apache.org/r/7588/#comment26650>

    Minor. Can you please follow the ordering of import statements?

src/java/org/apache/sqoop/manager/MySQLManager.java
<https://reviews.apache.org/r/7588/#comment26651>

    Same.

src/test/com/cloudera/sqoop/manager/JdbcMySQLExportTest.java
<https://reviews.apache.org/r/7588/#comment26653>

    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.
- Cheolsoo Park
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
>
>

> 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
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7588/
-----------------------------------------------------------

(Updated Oct. 17, 2012, 11:33 p.m.)
Review request for Sqoop.
Changes
-------

I've incorporated Cheolsoo's suggestions.
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 (updated)
-----

  src/docs/user/connectors.txt 930a4996e47a18a763bfbed59656cf8ea5cff05e
  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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7588/#review12555
-----------------------------------------------------------

Ship it!
Ship It!

- Cheolsoo Park
On Oct. 17, 2012, 11:33 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7588/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2012, 11:33 p.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/docs/user/connectors.txt 930a4996e47a18a763bfbed59656cf8ea5cff05e
>   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
>
>