> On June 28, 2014, 11:11 p.m., Jarek Cecho wrote: > > Looks good, nice work Abe, couple of comments:
Thanks for the review Jarcec. > On June 28, 2014, 11:11 p.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/tool/ImportTool.java, lines 1054-1055 > > <https://reviews.apache.org/r/23047/diff/2/?file=619737#file619737line1054> > > > > Shouldn't the conditions be other way around? > > > > E.g. if(mergeCol == NULL && incremetnalMode == DateLastModified) then > > bad else ok; In the case mergeCol == NULL && incrementalMode == DataLastModified, it should continue executing the method to attempt to "move" the previously generated temporary directory to its final destination. If the destination exists already, it will fail (similar to exceptions seen prior to this review). > On June 28, 2014, 11:11 p.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/tool/ImportTool.java, lines 453-454 > > <https://reviews.apache.org/r/23047/diff/2/?file=619737#file619737line453> > > > > Don't we also need to do clean up of the temporary directory? Like > > remove it or something. Sure. This could be a lot of data and is an intermediate step. > On June 28, 2014, 11:11 p.m., Jarek Cecho wrote: > > src/java/org/apache/sqoop/tool/ImportTool.java, line 34 > > <https://reviews.apache.org/r/23047/diff/2/?file=619737#file619737line34> > > > > Nit: Please don't use the asterisk import - always iterate over all > > imported classes. Indeed. - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23047/#review46944 ----------------------------------------------------------- On June 27, 2014, 7:31 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23047/ > ----------------------------------------------------------- > > (Updated June 27, 2014, 7:31 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1138 > https://issues.apache.org/jira/browse/SQOOP-1138 > > > Repository: sqoop-trunk > > > Description > ------- > > commit 7642758ec9d3e2d798bc18a73af2662783651594 > Author: Abraham Elmahrek <[email protected]> > Date: Thu Jun 26 14:48:31 2014 -0700 > > SQOOP-1138 incremental lastmodified should re-use output directory > > Added merge-key option for lastmodified. > If the target directory already exists, try to perform a merge. > Otherwise, just move the temporary directory to the target > directory. > > :100644 100644 6cbb873... bf654b5... M > src/java/org/apache/sqoop/tool/ImportTool.java > :100644 100644 8eadcdd... fd94552... M > src/test/com/cloudera/sqoop/TestIncrementalImport.java > > > Diffs > ----- > > src/java/org/apache/sqoop/tool/ImportTool.java 6cbb873 > src/test/com/cloudera/sqoop/TestIncrementalImport.java 8eadcdd > > Diff: https://reviews.apache.org/r/23047/diff/ > > > Testing > ------- > > ran ant tests with new test. > > > Thanks, > > Abraham Elmahrek > >
