> On 2011-09-06 07:45:14, jmhsieh wrote: > > src/test/com/cloudera/sqoop/manager/OracleExportTest.java, lines 276-277 > > <https://reviews.apache.org/r/1717/diff/2/?file=37992#file37992line276> > > > > Are you upserting the same values? would it make more sense to put new > > values in and verify them? > > > > Also, would it make sense to have some overlapping range to prove that > > some are updated and some are inserted?
The values go through the same code path twice. This would also test out both "insert" in first round and "update" in second round. > On 2011-09-06 07:45:14, jmhsieh wrote: > > src/java/com/cloudera/sqoop/manager/ConnManager.java, line 284 > > <https://reviews.apache.org/r/1717/diff/2/?file=37985#file37985line284> > > > > maybe you should have a '--update' and '--upsert' options instead of > > introducing modality? Boolean "--upsert" option was actually the very first design. The reason to go with a enum now is to allow flexibility for adding new values in the future (such as throwing exception for new rows.) so we won't paint ourselves into a corner. > On 2011-09-06 07:45:14, jmhsieh wrote: > > src/docs/user/export.txt, lines 55-59 > > <https://reviews.apache.org/r/1717/diff/2/?file=37983#file37983line55> > > > > which mode is default? > > > > maybe make it --export-mode with options 'update', 'insert', or > > 'upsert'? Or maybe just have --update, --insert and --upsert? Will clarify the default to be "updateonly". (This is mentioned in "sqoop-export.txt" though.) This proposal was also one of several designs considered. It was not chosen because it would introduce unnecessary and meaningless option combination (like "--export-mode insert" and "--update-key col" together) and "upsert" is essentially a "sub-mode" of update mode. > On 2011-09-06 07:45:14, jmhsieh wrote: > > src/docs/user/export.txt, lines 177-181 > > <https://reviews.apache.org/r/1717/diff/2/?file=37983#file37983line177> > > > > this is confusing. reword to be positive? > > > > I think 'mixed' mode means "insert or update" while "updateonly" means > > update only. maybe change 'mixed' to 'allowinsert'? Thanks, "allowinsert" sounds better (will use it in next patch). This paragraph is written following from the previous paragraph, so it would make more sense if continuing reading from previous paragraph. Even though, will reword it to make it clear by itself. - Bilung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1717/#review1761 ----------------------------------------------------------- On 2011-09-06 00:02:42, Bilung Lee wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1717/ > ----------------------------------------------------------- > > (Updated 2011-09-06 00:02:42) > > > Review request for Sqoop, Ahmed Radwan and Arvind Prabhakar. > > > Summary > ------- > > A new option is introduced to allow update records if they exist in table > already or to insert records if they do not exist yet. > > > This addresses bug SQOOP-327. > https://issues.apache.org/jira/browse/SQOOP-327 > > > Diffs > ----- > > src/docs/man/sqoop-export.txt 50052cc > src/docs/user/export.txt 4401c26 > src/java/com/cloudera/sqoop/SqoopOptions.java d07aecc > src/java/com/cloudera/sqoop/manager/ConnManager.java f5f5a4b > src/java/com/cloudera/sqoop/manager/OracleManager.java 1d08c4d > src/java/com/cloudera/sqoop/mapreduce/JdbcUpsertExportJob.java PRE-CREATION > src/java/com/cloudera/sqoop/mapreduce/OracleUpsertOutputFormat.java > PRE-CREATION > src/java/com/cloudera/sqoop/mapreduce/UpdateOutputFormat.java d5339d9 > src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java 879c7c8 > src/java/com/cloudera/sqoop/tool/ExportTool.java d156eeb > src/test/com/cloudera/sqoop/manager/OracleExportTest.java 12858d7 > > Diff: https://reviews.apache.org/r/1717/diff > > > Testing > ------- > > > Thanks, > > Bilung > >
