You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Boglarka Egyed <bo...@apache.org> on 2018/08/28 11:56:36 UTC
Review Request 68536: SQOOP-3368: Add fail-fast scenarios to S3
incremental import use cases without --temporary-rootdir option
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68536/
-----------------------------------------------------------
Review request for Sqoop, daniel voros, Fero Szabo, Nguyen Truong, and Szabolcs Vasas.
Bugs: SQOOP-3368
https://issues.apache.org/jira/browse/SQOOP-3368
Repository: sqoop-trunk
Description
-------
The current implementation of Sqoop handles HDFS as a default filesystem, i.e. it creates temporary directories on HDFS in case of incremental append or merge imports. To make these incremental import use cases work with S3 the user needs to set the --temporary-rootdir to an S3 location properly.
Diffs
-----
src/java/org/apache/sqoop/tool/ImportTool.java 139733732d2a28d171568b9118c98a47a3d2fc50
src/test/org/apache/sqoop/tool/TestS3IncrementalImportOptionValidations.java PRE-CREATION
Diff: https://reviews.apache.org/r/68536/diff/1/
Testing
-------
ant clean test
./gradlew test
Thanks,
Boglarka Egyed
Re: Review Request 68536: SQOOP-3368: Add fail-fast scenarios to S3
incremental import use cases without --temporary-rootdir option
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68536/#review208043
-----------------------------------------------------------
Hi Bogi,
Thanks for submitting this patch, it fills an important gap in Sqoop's S3 support.
Please see my below findings:
src/java/org/apache/sqoop/tool/ImportTool.java
Lines 1170 (patched)
<https://reviews.apache.org/r/68536/#comment291788>
Can we somehow simplify and merge these 2 if statements?
Ideas:
- since IncrementalMode has only 3 possible values checking if options.getIncrementalMode() != SqoopOptions.IncrementalMode.None might be enough
- by using org.apache.commons.lang3.StringUtils#contains we could avoid null checks
src/test/org/apache/sqoop/tool/TestS3IncrementalImportOptionValidations.java
Lines 97 (patched)
<https://reviews.apache.org/r/68536/#comment291789>
Just for the sake of completeness we could add test case(s) to verify if no exception is thrown when everything is OK.
- Szabolcs Vasas
On Aug. 28, 2018, 11:56 a.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68536/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2018, 11:56 a.m.)
>
>
> Review request for Sqoop, daniel voros, Fero Szabo, Nguyen Truong, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3368
> https://issues.apache.org/jira/browse/SQOOP-3368
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> The current implementation of Sqoop handles HDFS as a default filesystem, i.e. it creates temporary directories on HDFS in case of incremental append or merge imports. To make these incremental import use cases work with S3 the user needs to set the --temporary-rootdir to an S3 location properly.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/tool/ImportTool.java 139733732d2a28d171568b9118c98a47a3d2fc50
> src/test/org/apache/sqoop/tool/TestS3IncrementalImportOptionValidations.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68536/diff/1/
>
>
> Testing
> -------
>
> ant clean test
> ./gradlew test
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 68536: SQOOP-3368: Add fail-fast scenarios to S3
incremental import use cases without --temporary-rootdir option
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68536/#review208123
-----------------------------------------------------------
Ship it!
Hi Bogi,
Thanks for improving the patch, I have ran the tests, everything looks good, let's ship it!
- Szabolcs Vasas
On Aug. 30, 2018, 9:59 a.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68536/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2018, 9:59 a.m.)
>
>
> Review request for Sqoop, daniel voros, Fero Szabo, Nguyen Truong, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3368
> https://issues.apache.org/jira/browse/SQOOP-3368
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> The current implementation of Sqoop handles HDFS as a default filesystem, i.e. it creates temporary directories on HDFS in case of incremental append or merge imports. To make these incremental import use cases work with S3 the user needs to set the --temporary-rootdir to an S3 location properly.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/tool/ImportTool.java 139733732d2a28d171568b9118c98a47a3d2fc50
> src/test/org/apache/sqoop/tool/TestS3IncrementalImportOptionValidations.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68536/diff/3/
>
>
> Testing
> -------
>
> ant clean test
> ./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 68536: SQOOP-3368: Add fail-fast scenarios to S3
incremental import use cases without --temporary-rootdir option
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68536/
-----------------------------------------------------------
(Updated Aug. 30, 2018, 9:59 a.m.)
Review request for Sqoop, daniel voros, Fero Szabo, Nguyen Truong, and Szabolcs Vasas.
Changes
-------
Changed to startsWith() usage, looking for s3a uri scheme intead of "s3" only
Bugs: SQOOP-3368
https://issues.apache.org/jira/browse/SQOOP-3368
Repository: sqoop-trunk
Description
-------
The current implementation of Sqoop handles HDFS as a default filesystem, i.e. it creates temporary directories on HDFS in case of incremental append or merge imports. To make these incremental import use cases work with S3 the user needs to set the --temporary-rootdir to an S3 location properly.
Diffs (updated)
-----
src/java/org/apache/sqoop/tool/ImportTool.java 139733732d2a28d171568b9118c98a47a3d2fc50
src/test/org/apache/sqoop/tool/TestS3IncrementalImportOptionValidations.java PRE-CREATION
Diff: https://reviews.apache.org/r/68536/diff/3/
Changes: https://reviews.apache.org/r/68536/diff/2-3/
Testing
-------
ant clean test
./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
Thanks,
Boglarka Egyed
Re: Review Request 68536: SQOOP-3368: Add fail-fast scenarios to S3
incremental import use cases without --temporary-rootdir option
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68536/
-----------------------------------------------------------
(Updated Aug. 29, 2018, 11:11 a.m.)
Review request for Sqoop, daniel voros, Fero Szabo, Nguyen Truong, and Szabolcs Vasas.
Bugs: SQOOP-3368
https://issues.apache.org/jira/browse/SQOOP-3368
Repository: sqoop-trunk
Description
-------
The current implementation of Sqoop handles HDFS as a default filesystem, i.e. it creates temporary directories on HDFS in case of incremental append or merge imports. To make these incremental import use cases work with S3 the user needs to set the --temporary-rootdir to an S3 location properly.
Diffs
-----
src/java/org/apache/sqoop/tool/ImportTool.java 139733732d2a28d171568b9118c98a47a3d2fc50
src/test/org/apache/sqoop/tool/TestS3IncrementalImportOptionValidations.java PRE-CREATION
Diff: https://reviews.apache.org/r/68536/diff/2/
Testing (updated)
-------
ant clean test
./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
Thanks,
Boglarka Egyed
Re: Review Request 68536: SQOOP-3368: Add fail-fast scenarios to S3
incremental import use cases without --temporary-rootdir option
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68536/
-----------------------------------------------------------
(Updated Aug. 29, 2018, 11:10 a.m.)
Review request for Sqoop, daniel voros, Fero Szabo, Nguyen Truong, and Szabolcs Vasas.
Changes
-------
Simplified validation condition, added poitive path test case
Bugs: SQOOP-3368
https://issues.apache.org/jira/browse/SQOOP-3368
Repository: sqoop-trunk
Description
-------
The current implementation of Sqoop handles HDFS as a default filesystem, i.e. it creates temporary directories on HDFS in case of incremental append or merge imports. To make these incremental import use cases work with S3 the user needs to set the --temporary-rootdir to an S3 location properly.
Diffs (updated)
-----
src/java/org/apache/sqoop/tool/ImportTool.java 139733732d2a28d171568b9118c98a47a3d2fc50
src/test/org/apache/sqoop/tool/TestS3IncrementalImportOptionValidations.java PRE-CREATION
Diff: https://reviews.apache.org/r/68536/diff/2/
Changes: https://reviews.apache.org/r/68536/diff/1-2/
Testing
-------
ant clean test
./gradlew test
Thanks,
Boglarka Egyed