sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Abraham Elmahrek" <...@cloudera.com>
Subject Re: Review Request 23047: SQOOP-1138 incremental lastmodified should re-use output directory
Date Mon, 30 Jun 2014 05:07:43 GMT


> 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 <abraham@elmahrek.com>
> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message