----------------------------------------------------------- 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 > >
