sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Fero Szabo via Review Board <nore...@reviews.apache.org>
Subject Re: Review Request 37353: Support snappy compression in Sqoop Import with HCatalog.The Jira is SQOOP-2331
Date Fri, 10 Aug 2018 13:43:38 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37353/#review207078
-----------------------------------------------------------



Hi Shashank,

Other than the minor issue with the default value of the compression codec, this change looks
ok to me. However, since these are new features in Sqoop, can you please add a few tests for
them?

You can find similar tests to what you'll need in HCatalogImportTest.

The best way would be to add a new test class (it could be called HCatalogImportWithCompressionTest)
and also utilize the ArgumentArrayBuilder class. You can find an example on how to use the
builder in one of the more recent tests, such as the newly added S3 tests (TestS3AvroImport.java
which calls into S3TestUtils.java). Anyway, please make sure to test both file formats. If
you can think of multiple testcases that make sense, that's also welcome! 

I will be on vacation till the August 22, but hopefully other members of the community will
help you out if you've any questions.

(Just for the record, I've also ran the unit and 3rd party tests, and didn't find any issues.)


src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java
Lines 900-904 (patched)
<https://reviews.apache.org/r/37353/#comment290271>

    The default codec is gzip. However, if the codecName is not specified by the user, execution
will never reach this point in the code. Please revise.



src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java
Lines 910 (patched)
<https://reviews.apache.org/r/37353/#comment290269>

    nit: missing space before else



src/java/org/apache/sqoop/tool/ImportTool.java
Lines 1178-1179 (patched)
<https://reviews.apache.org/r/37353/#comment290270>

    I think there is a mismatch between the validation and the setup logic. 
    
    Here in the validation there is no default value, but an exception is thrown. In the setup
logic, if codec is not specified GZIP is used as a default. 
    
    I believe if the codec is not set, we should log a warning, and tell the user that the
default will be used i.e. GZIP (or change it to SNAPPY if that's preferable).


- Fero Szabo


On Aug. 10, 2018, 7:29 a.m., Shashank Tandon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37353/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2018, 7:29 a.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed, Fero Szabo, Szabolcs Vasas, and Venkat Ranganathan.
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> Apache Sqoop does not compress with --compress option with --hcatalog-table.It also does
not support option --compression-codec snappy. Will add Snappy compression support in Apache
Sqoop. When a user will try to use --compress, then it will use the by default compression
i.e. GZIP. otherwise If user provide option --compress --compression-codec snappy then it
will compress into snappy format.
> 
> 
> Diffs
> -----
> 
>   src/docs/user/hcatalog.txt 2ae1d54d 
>   src/java/org/apache/sqoop/config/ConfigurationConstants.java 3724f250 
>   src/java/org/apache/sqoop/io/CodecMap.java d5796188 
>   src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java 784b5f2a 
>   src/java/org/apache/sqoop/tool/ImportTool.java ccded652 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabb 
> 
> 
> Diff: https://reviews.apache.org/r/37353/diff/3/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> File Attachments
> ----------------
> 
> SQOOP-2331_2.patch
>   https://reviews.apache.org/media/uploaded/files/2018/07/24/d474a04e-fe57-4c06-a066-0b70befcd29d__SQOOP-2331_2.patch
> SQOOP-2331_2.patch
>   https://reviews.apache.org/media/uploaded/files/2018/08/10/c4004353-2230-4ef9-9ccc-c4934314f548__SQOOP-2331_3.patch
> SQOOP-2331_3.patch
>   https://reviews.apache.org/media/uploaded/files/2018/08/10/bf01aa4e-b9d2-43ac-bb5a-80a4787ad1a5__SQOOP-2331_3.patch
> 
> 
> Thanks,
> 
> Shashank Tandon
> 
>


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