You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Raghav Gautam <ra...@gmail.com> on 2013/05/31 22:37:00 UTC
Review Request: Review request for SQOOP-883: Adds a --delete-target-dir
option
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11517/
-----------------------------------------------------------
Review request for Sqoop.
Summary (updated)
-----------------
Review request for SQOOP-883: Adds a --delete-target-dir option
Description (updated)
-------
After first import, re-importing to a directory fails because the directory already exists. This patch tries to fix that by providing a --delete-target-dir option. This option will delete the target directory of the import before running the import if it exists.
Diffs (updated)
-----
src/docs/user/import.txt ee10b1c
src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java a5f72f7
src/java/org/apache/sqoop/SqoopOptions.java f18d43e
src/java/org/apache/sqoop/tool/BaseSqoopTool.java 42f521f
src/java/org/apache/sqoop/tool/ImportTool.java 2627726
src/test/com/cloudera/sqoop/TestIncrementalImport.java cab77ec
src/test/com/cloudera/sqoop/mapreduce/TestImportJob.java 6ab3b82
Diff: https://reviews.apache.org/r/11517/diff/
Testing (updated)
-------
Added new test for the option.
All the tests pass.
Thanks,
Raghav Gautam
Re: Review Request: Review request for SQOOP-883: Adds a --delete-target-dir
option
Posted by Raghav Gautam <ra...@gmail.com>.
> On June 2, 2013, 2:07 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/tool/BaseSqoopTool.java, line 830
> > <https://reviews.apache.org/r/11517/diff/1/?file=299656#file299656line830>
> >
> > Nit: This change do not seem to be relevant for this patch.
Removed. Thanks for pointing it out.
> On June 2, 2013, 2:07 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/tool/BaseSqoopTool.java, line 1054
> > <https://reviews.apache.org/r/11517/diff/1/?file=299656#file299656line1054>
> >
> > Nit: This change do not seem to be relevant for this patch.
Removed. Thanks for pointing it out.
> On June 2, 2013, 2:07 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/tool/BaseSqoopTool.java, line 1060
> > <https://reviews.apache.org/r/11517/diff/1/?file=299656#file299656line1060>
> >
> > Nit: This change do not seem to be relevant for this patch.
Removed. Thanks for pointing it out.
> On June 2, 2013, 2:07 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/tool/ImportTool.java, lines 720-724
> > <https://reviews.apache.org/r/11517/diff/1/?file=299657#file299657line720>
> >
> > It seems to me that using incremental import and this new argument should be mutually exclusive as it do not make sense to do incremental import into directory that you will remove at the start of the execution, right?
Fixed. Thanks for pointing it out. I have also changed tests to reflect this.
- Raghav
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11517/#review21294
-----------------------------------------------------------
On June 6, 2013, 3:56 a.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11517/
> -----------------------------------------------------------
>
> (Updated June 6, 2013, 3:56 a.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> After first import, re-importing to a directory fails because the directory already exists. This patch tries to fix that by providing a --delete-target-dir option. This option will delete the target directory of the import before running the import if it exists.
>
>
> Diffs
> -----
>
> src/docs/user/import.txt ee10b1c
> src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java a5f72f7
> src/java/org/apache/sqoop/SqoopOptions.java f18d43e
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 42f521f
> src/java/org/apache/sqoop/tool/ImportTool.java 2627726
> src/test/com/cloudera/sqoop/TestSqoopOptions.java c78cd87
> src/test/com/cloudera/sqoop/mapreduce/TestImportJob.java 6ab3b82
>
> Diff: https://reviews.apache.org/r/11517/diff/
>
>
> Testing
> -------
>
> Added new test for the option.
> All the tests pass.
>
>
> Thanks,
>
> Raghav Gautam
>
>
Re: Review Request: Review request for SQOOP-883: Adds a --delete-target-dir
option
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11517/#review21294
-----------------------------------------------------------
Hi Raghav,
thank you very much for taking this one! I do have couple of nits:
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/11517/#comment44174>
Nit: This change do not seem to be relevant for this patch.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/11517/#comment44175>
Nit: This change do not seem to be relevant for this patch.
src/java/org/apache/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/11517/#comment44176>
Nit: This change do not seem to be relevant for this patch.
src/java/org/apache/sqoop/tool/ImportTool.java
<https://reviews.apache.org/r/11517/#comment44177>
It seems to me that using incremental import and this new argument should be mutually exclusive as it do not make sense to do incremental import into directory that you will remove at the start of the execution, right?
Jarcec
- Jarek Cecho
On May 31, 2013, 8:36 p.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11517/
> -----------------------------------------------------------
>
> (Updated May 31, 2013, 8:36 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> After first import, re-importing to a directory fails because the directory already exists. This patch tries to fix that by providing a --delete-target-dir option. This option will delete the target directory of the import before running the import if it exists.
>
>
> Diffs
> -----
>
> src/docs/user/import.txt ee10b1c
> src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java a5f72f7
> src/java/org/apache/sqoop/SqoopOptions.java f18d43e
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 42f521f
> src/java/org/apache/sqoop/tool/ImportTool.java 2627726
> src/test/com/cloudera/sqoop/TestIncrementalImport.java cab77ec
> src/test/com/cloudera/sqoop/mapreduce/TestImportJob.java 6ab3b82
>
> Diff: https://reviews.apache.org/r/11517/diff/
>
>
> Testing
> -------
>
> Added new test for the option.
> All the tests pass.
>
>
> Thanks,
>
> Raghav Gautam
>
>
Re: Review Request: Review request for SQOOP-883: Adds a --delete-target-dir
option
Posted by Venkat Ranganathan <n....@live.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11517/#review21677
-----------------------------------------------------------
Ship it!
Ship It!
- Venkat Ranganathan
On June 10, 2013, 9:29 p.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11517/
> -----------------------------------------------------------
>
> (Updated June 10, 2013, 9:29 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> After first import, re-importing to a directory fails because the directory already exists. This patch tries to fix that by providing a --delete-target-dir option. This option will delete the target directory of the import before running the import if it exists.
>
>
> Diffs
> -----
>
> src/docs/user/import.txt 4a9a316
> src/java/org/apache/sqoop/SqoopOptions.java 4be6a6a
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 01a55e5
> src/java/org/apache/sqoop/tool/ImportTool.java 1c57503
> src/test/com/cloudera/sqoop/TestSqoopOptions.java c78cd87
> src/test/com/cloudera/sqoop/mapreduce/TestImportJob.java 6ab3b82
>
> Diff: https://reviews.apache.org/r/11517/diff/
>
>
> Testing
> -------
>
> Added new test for the option.
> All the tests pass.
>
>
> Thanks,
>
> Raghav Gautam
>
>
Re: Review Request: Review request for SQOOP-883: Adds a --delete-target-dir
option
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11517/#review21728
-----------------------------------------------------------
Ship it!
Will commit as soon as Apache Git server will be back up.
- Jarek Cecho
On June 10, 2013, 9:29 p.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11517/
> -----------------------------------------------------------
>
> (Updated June 10, 2013, 9:29 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> After first import, re-importing to a directory fails because the directory already exists. This patch tries to fix that by providing a --delete-target-dir option. This option will delete the target directory of the import before running the import if it exists.
>
>
> Diffs
> -----
>
> src/docs/user/import.txt 4a9a316
> src/java/org/apache/sqoop/SqoopOptions.java 4be6a6a
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 01a55e5
> src/java/org/apache/sqoop/tool/ImportTool.java 1c57503
> src/test/com/cloudera/sqoop/TestSqoopOptions.java c78cd87
> src/test/com/cloudera/sqoop/mapreduce/TestImportJob.java 6ab3b82
>
> Diff: https://reviews.apache.org/r/11517/diff/
>
>
> Testing
> -------
>
> Added new test for the option.
> All the tests pass.
>
>
> Thanks,
>
> Raghav Gautam
>
>
Re: Review Request: Review request for SQOOP-883: Adds a --delete-target-dir
option
Posted by Raghav Gautam <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11517/
-----------------------------------------------------------
(Updated June 10, 2013, 9:29 p.m.)
Review request for Sqoop.
Changes
-------
Rebased the patch and added checks for the directory content.
Description
-------
After first import, re-importing to a directory fails because the directory already exists. This patch tries to fix that by providing a --delete-target-dir option. This option will delete the target directory of the import before running the import if it exists.
Diffs (updated)
-----
src/docs/user/import.txt 4a9a316
src/java/org/apache/sqoop/SqoopOptions.java 4be6a6a
src/java/org/apache/sqoop/tool/BaseSqoopTool.java 01a55e5
src/java/org/apache/sqoop/tool/ImportTool.java 1c57503
src/test/com/cloudera/sqoop/TestSqoopOptions.java c78cd87
src/test/com/cloudera/sqoop/mapreduce/TestImportJob.java 6ab3b82
Diff: https://reviews.apache.org/r/11517/diff/
Testing
-------
Added new test for the option.
All the tests pass.
Thanks,
Raghav Gautam
Re: Review Request: Review request for SQOOP-883: Adds a --delete-target-dir
option
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11517/#review21626
-----------------------------------------------------------
Ship it!
Hi Raghav,
the patch do not longer applies on trunk, so would you mind rebasing it? Otherwise I do have two small nits:
src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java
<https://reviews.apache.org/r/11517/#comment44692>
Nit: Please don't change files in com.cloudera modules. They are provided only for backward compatibility with cope created for pre-1.4 versions and thus should not be altered anymore.
src/test/com/cloudera/sqoop/mapreduce/TestImportJob.java
<https://reviews.apache.org/r/11517/#comment44693>
Nit: Do you think that it would make sense to also validate that the content of imported directory is as expected or do you think that it's not needed in this case?
Jarcec
- Jarek Cecho
On June 6, 2013, 3:56 a.m., Raghav Gautam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11517/
> -----------------------------------------------------------
>
> (Updated June 6, 2013, 3:56 a.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> After first import, re-importing to a directory fails because the directory already exists. This patch tries to fix that by providing a --delete-target-dir option. This option will delete the target directory of the import before running the import if it exists.
>
>
> Diffs
> -----
>
> src/docs/user/import.txt ee10b1c
> src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java a5f72f7
> src/java/org/apache/sqoop/SqoopOptions.java f18d43e
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 42f521f
> src/java/org/apache/sqoop/tool/ImportTool.java 2627726
> src/test/com/cloudera/sqoop/TestSqoopOptions.java c78cd87
> src/test/com/cloudera/sqoop/mapreduce/TestImportJob.java 6ab3b82
>
> Diff: https://reviews.apache.org/r/11517/diff/
>
>
> Testing
> -------
>
> Added new test for the option.
> All the tests pass.
>
>
> Thanks,
>
> Raghav Gautam
>
>
Re: Review Request: Review request for SQOOP-883: Adds a --delete-target-dir
option
Posted by Raghav Gautam <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11517/
-----------------------------------------------------------
(Updated June 6, 2013, 3:56 a.m.)
Review request for Sqoop.
Changes
-------
Updated diff incorporates all the comments.
Description
-------
After first import, re-importing to a directory fails because the directory already exists. This patch tries to fix that by providing a --delete-target-dir option. This option will delete the target directory of the import before running the import if it exists.
Diffs (updated)
-----
src/docs/user/import.txt ee10b1c
src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java a5f72f7
src/java/org/apache/sqoop/SqoopOptions.java f18d43e
src/java/org/apache/sqoop/tool/BaseSqoopTool.java 42f521f
src/java/org/apache/sqoop/tool/ImportTool.java 2627726
src/test/com/cloudera/sqoop/TestSqoopOptions.java c78cd87
src/test/com/cloudera/sqoop/mapreduce/TestImportJob.java 6ab3b82
Diff: https://reviews.apache.org/r/11517/diff/
Testing
-------
Added new test for the option.
All the tests pass.
Thanks,
Raghav Gautam