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/09/13 18:35:17 UTC
Review Request 68712: SQOOP-3376: Test import into external Hive table
backed by S3
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/
-----------------------------------------------------------
Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
Bugs: SQOOP-3376
https://issues.apache.org/jira/browse/SQOOP-3376
Repository: sqoop-trunk
Description
-------
Testing the Text and Parquet imports into an external Hive table backed by S3.
Diffs
-----
src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableParquetImport.java PRE-CREATION
src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableTextImport.java PRE-CREATION
src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
Diff: https://reviews.apache.org/r/68712/diff/1/
Testing
-------
./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
Thanks,
Boglarka Egyed
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Boglarka Egyed <bo...@apache.org>.
> On Sept. 20, 2018, 10:29 a.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableTextImport.java
> > Lines 112 (patched)
> > <https://reviews.apache.org/r/68712/diff/2/?file=2090098#file2090098line112>
> >
> > This assert could be simplified if we introduce a convenience method to HiveServer2TestUtils, which would return the rows in CSV. For example:
> >
> > public List<String> loadCsvRowsFromTable(String tableName) {
> > return loadRawRowsFromTable(tableName).stream()
> > .map(list -> StringUtils.join(list, ","))
> > .collect(toList());
> > }
> >
> > If we have this method we can just use Assert.assertEquals to compare the 2 lists.
You are totally right, thanks for the suggestion! I have changed to a parameterized test case instead.
- Boglarka
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/#review208794
-----------------------------------------------------------
On Sept. 24, 2018, 11:12 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68712/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2018, 11:12 p.m.)
>
>
> Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3376
> https://issues.apache.org/jira/browse/SQOOP-3376
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Testing the Text and Parquet imports into an external Hive table backed by S3.
>
>
> Diffs
> -----
>
> src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java PRE-CREATION
> src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java 799370816cccda7578d7c64add6e283d3123e1c8
> src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
>
>
> Diff: https://reviews.apache.org/r/68712/diff/3/
>
>
> Testing
> -------
>
> ./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/#review208794
-----------------------------------------------------------
Hi Bogi,
Thank you for improving the patch, it is much more concise now.
However I think we could make it more concise by introducing parametrization since the 2 new test cases are almost the same the only real difference is the file format.
What do you think?
src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableTextImport.java
Lines 112 (patched)
<https://reviews.apache.org/r/68712/#comment292991>
This assert could be simplified if we introduce a convenience method to HiveServer2TestUtils, which would return the rows in CSV. For example:
public List<String> loadCsvRowsFromTable(String tableName) {
return loadRawRowsFromTable(tableName).stream()
.map(list -> StringUtils.join(list, ","))
.collect(toList());
}
If we have this method we can just use Assert.assertEquals to compare the 2 lists.
- Szabolcs Vasas
On Sept. 18, 2018, 2:24 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68712/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2018, 2:24 p.m.)
>
>
> Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3376
> https://issues.apache.org/jira/browse/SQOOP-3376
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Testing the Text and Parquet imports into an external Hive table backed by S3.
>
>
> Diffs
> -----
>
> src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableParquetImport.java PRE-CREATION
> src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableTextImport.java PRE-CREATION
> src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
>
>
> Diff: https://reviews.apache.org/r/68712/diff/2/
>
>
> Testing
> -------
>
> ./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Boglarka Egyed <bo...@apache.org>.
> On Oct. 10, 2018, 11:26 a.m., Szabolcs Vasas wrote:
> > Hi Bogi,
> >
> > Thank you for improving the patch, it is much more concise now.
> > I have ran the tests successfully however I noticed that when I run the tests without the S3 generator command the new test fails and does not get skipped.
> > I am not sure but the ExpectedException could have something to do with the failure.
> > Can you please check this?
Hi Szabolcs,
Thanks, I missed to check this scenario with the new test case. Skip logic works as expected but a null check was missing from the After method.
Thank you,
Bogi
- Boglarka
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/#review209401
-----------------------------------------------------------
On Oct. 10, 2018, 11:59 a.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68712/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2018, 11:59 a.m.)
>
>
> Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3376
> https://issues.apache.org/jira/browse/SQOOP-3376
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Testing the Text and Parquet imports into an external Hive table backed by S3.
>
>
> Diffs
> -----
>
> src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java PRE-CREATION
> src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java 799370816cccda7578d7c64add6e283d3123e1c8
> src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
>
>
> Diff: https://reviews.apache.org/r/68712/diff/5/
>
>
> Testing
> -------
>
> ./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/#review209401
-----------------------------------------------------------
Hi Bogi,
Thank you for improving the patch, it is much more concise now.
I have ran the tests successfully however I noticed that when I run the tests without the S3 generator command the new test fails and does not get skipped.
I am not sure but the ExpectedException could have something to do with the failure.
Can you please check this?
- Szabolcs Vasas
On Oct. 4, 2018, 4:13 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68712/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2018, 4:13 p.m.)
>
>
> Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3376
> https://issues.apache.org/jira/browse/SQOOP-3376
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Testing the Text and Parquet imports into an external Hive table backed by S3.
>
>
> Diffs
> -----
>
> src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java PRE-CREATION
> src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java 799370816cccda7578d7c64add6e283d3123e1c8
> src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
>
>
> Diff: https://reviews.apache.org/r/68712/diff/4/
>
>
> Testing
> -------
>
> ./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/#review209404
-----------------------------------------------------------
Ship it!
Ship It!
- Szabolcs Vasas
On Oct. 10, 2018, 11:59 a.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68712/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2018, 11:59 a.m.)
>
>
> Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3376
> https://issues.apache.org/jira/browse/SQOOP-3376
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Testing the Text and Parquet imports into an external Hive table backed by S3.
>
>
> Diffs
> -----
>
> src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java PRE-CREATION
> src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java 799370816cccda7578d7c64add6e283d3123e1c8
> src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
>
>
> Diff: https://reviews.apache.org/r/68712/diff/5/
>
>
> Testing
> -------
>
> ./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/
-----------------------------------------------------------
(Updated Oct. 10, 2018, 11:59 a.m.)
Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
Changes
-------
Removed unnecessary ExpectedException rule, added null check for hiveMiniCluster stopping in After method.
Bugs: SQOOP-3376
https://issues.apache.org/jira/browse/SQOOP-3376
Repository: sqoop-trunk
Description
-------
Testing the Text and Parquet imports into an external Hive table backed by S3.
Diffs (updated)
-----
src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java PRE-CREATION
src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java 799370816cccda7578d7c64add6e283d3123e1c8
src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
Diff: https://reviews.apache.org/r/68712/diff/5/
Changes: https://reviews.apache.org/r/68712/diff/4-5/
Testing
-------
./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
Thanks,
Boglarka Egyed
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/
-----------------------------------------------------------
(Updated Oct. 4, 2018, 4:13 p.m.)
Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
Changes
-------
Fixed typo
Bugs: SQOOP-3376
https://issues.apache.org/jira/browse/SQOOP-3376
Repository: sqoop-trunk
Description
-------
Testing the Text and Parquet imports into an external Hive table backed by S3.
Diffs (updated)
-----
src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java PRE-CREATION
src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java 799370816cccda7578d7c64add6e283d3123e1c8
src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
Diff: https://reviews.apache.org/r/68712/diff/4/
Changes: https://reviews.apache.org/r/68712/diff/3-4/
Testing
-------
./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
Thanks,
Boglarka Egyed
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Boglarka Egyed <bo...@apache.org>.
> On Oct. 4, 2018, 3:42 p.m., Fero Szabo wrote:
> > Lgtm!
> >
> > It is interesting to see that you ran into the problem that parameterized tests don't support multiple dimensions!
> >
> > In any case, I like the tests as they are now, they are concise enough and descriptive enough.
> >
> > My only concern is documentation, and that it should also cover the kinks and quirks. But I see you've filed a separate Jira for that.
Thanks Fero for taking a look! Yes, all documentation update will be covered in SQOOP-3384.
- Boglarka
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/#review209226
-----------------------------------------------------------
On Oct. 4, 2018, 4:13 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68712/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2018, 4:13 p.m.)
>
>
> Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3376
> https://issues.apache.org/jira/browse/SQOOP-3376
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Testing the Text and Parquet imports into an external Hive table backed by S3.
>
>
> Diffs
> -----
>
> src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java PRE-CREATION
> src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java 799370816cccda7578d7c64add6e283d3123e1c8
> src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
>
>
> Diff: https://reviews.apache.org/r/68712/diff/4/
>
>
> Testing
> -------
>
> ./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Fero Szabo via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/#review209226
-----------------------------------------------------------
Ship it!
Lgtm!
It is interesting to see that you ran into the problem that parameterized tests don't support multiple dimensions!
In any case, I like the tests as they are now, they are concise enough and descriptive enough.
My only concern is documentation, and that it should also cover the kinks and quirks. But I see you've filed a separate Jira for that.
src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java
Lines 100 (patched)
<https://reviews.apache.org/r/68712/#comment293541>
Typo :) e missing from TestCase
src/test/org/apache/sqoop/testutil/S3TestUtils.java
Lines 136-138 (original), 153-155 (patched)
<https://reviews.apache.org/r/68712/#comment293542>
This sounds like something we should also mention in the user guide!
- Fero Szabo
On Sept. 24, 2018, 11:12 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68712/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2018, 11:12 p.m.)
>
>
> Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-3376
> https://issues.apache.org/jira/browse/SQOOP-3376
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> Testing the Text and Parquet imports into an external Hive table backed by S3.
>
>
> Diffs
> -----
>
> src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java PRE-CREATION
> src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java 799370816cccda7578d7c64add6e283d3123e1c8
> src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
>
>
> Diff: https://reviews.apache.org/r/68712/diff/3/
>
>
> Testing
> -------
>
> ./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/
-----------------------------------------------------------
(Updated Sept. 24, 2018, 11:12 p.m.)
Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
Changes
-------
Made parameterized test for TextFile and ParquetFile formats
Bugs: SQOOP-3376
https://issues.apache.org/jira/browse/SQOOP-3376
Repository: sqoop-trunk
Description
-------
Testing the Text and Parquet imports into an external Hive table backed by S3.
Diffs (updated)
-----
src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java PRE-CREATION
src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java 799370816cccda7578d7c64add6e283d3123e1c8
src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
Diff: https://reviews.apache.org/r/68712/diff/3/
Changes: https://reviews.apache.org/r/68712/diff/2-3/
Testing
-------
./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
Thanks,
Boglarka Egyed
Re: Review Request 68712: SQOOP-3376: Test import into external Hive
table backed by S3
Posted by Boglarka Egyed <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/
-----------------------------------------------------------
(Updated Sept. 18, 2018, 2:24 p.m.)
Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
Changes
-------
Removed unnecessary Kerberos usage, changed to verification of Hive results, rationalized output directory structure
Bugs: SQOOP-3376
https://issues.apache.org/jira/browse/SQOOP-3376
Repository: sqoop-trunk
Description
-------
Testing the Text and Parquet imports into an external Hive table backed by S3.
Diffs (updated)
-----
src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableParquetImport.java PRE-CREATION
src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableTextImport.java PRE-CREATION
src/test/org/apache/sqoop/testutil/S3TestUtils.java 0e6ef5bf001797aa70a7ad50d261c6fd384222fe
Diff: https://reviews.apache.org/r/68712/diff/2/
Changes: https://reviews.apache.org/r/68712/diff/1-2/
Testing
-------
./gradlew test -Ds3.bucket.url=<bucket-url> -Ds3.generator.command=<credential-generator-command>
Thanks,
Boglarka Egyed