You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/09/18 03:42:18 UTC
[GitHub] [iceberg] JonasJ-ap opened a new pull request, #5784: AWS: update AWS Integration Test to fix false positives
JonasJ-ap opened a new pull request, #5784:
URL: https://github.com/apache/iceberg/pull/5784
The AWS Integration Test gives false positive errors if users use buckets in regions other than `us-east-1`. This PR updates the tests to accommodate the different region setting and eliminate false positive errors
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5784: AWS: update AWS Integration Test to fix false positives
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5784:
URL: https://github.com/apache/iceberg/pull/5784#discussion_r974863192
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/GlueTestBase.java:
##########
@@ -79,18 +79,18 @@ public static void beforeClass() {
S3FileIO fileIO = new S3FileIO(clientFactory::s3);
glueCatalog = new GlueCatalog();
glueCatalog.initialize(catalogName, testBucketPath, new AwsProperties(), glue,
- LockManagers.defaultLockManager(), fileIO);
+ LockManagers.defaultLockManager(), fileIO, ImmutableMap.of());
Review Comment:
Use this [initialize()](https://github.com/apache/iceberg/blob/b8a796effdca02108b2af005f977cbeb0d9cd718/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java#L160) in the test ensures all fields of `GlueCatalog` are initialized. Same to the other related changes in the integration tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #5784: AWS: update AWS Integration Test to fix false positives
Posted by GitBox <gi...@apache.org>.
jackye1995 merged PR #5784:
URL: https://github.com/apache/iceberg/pull/5784
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5784: AWS: update AWS Integration Test to fix false positives
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5784:
URL: https://github.com/apache/iceberg/pull/5784#discussion_r973655330
##########
aws/src/integration/java/org/apache/iceberg/aws/TestAssumeRoleAwsClientFactory.java:
##########
@@ -75,7 +75,7 @@ public void before() {
assumeRoleProperties = Maps.newHashMap();
assumeRoleProperties.put(AwsProperties.CLIENT_FACTORY, AssumeRoleAwsClientFactory.class.getName());
assumeRoleProperties.put(AwsProperties.HTTP_CLIENT_TYPE, AwsProperties.HTTP_CLIENT_TYPE_APACHE);
- assumeRoleProperties.put(AwsProperties.CLIENT_ASSUME_ROLE_REGION, "us-east-1");
+ assumeRoleProperties.put(AwsProperties.CLIENT_ASSUME_ROLE_REGION, AwsIntegTestUtil.testRegion());
Review Comment:
If the bucket is created in a different region, I think we should use the region specified by the user to perform integration tests. Otherwise, we may get a 400 bad request error when sending a request from `us-east-1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5784: AWS: update AWS Integration Test to fix false positives
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5784:
URL: https://github.com/apache/iceberg/pull/5784#discussion_r974863192
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/GlueTestBase.java:
##########
@@ -79,18 +79,18 @@ public static void beforeClass() {
S3FileIO fileIO = new S3FileIO(clientFactory::s3);
glueCatalog = new GlueCatalog();
glueCatalog.initialize(catalogName, testBucketPath, new AwsProperties(), glue,
- LockManagers.defaultLockManager(), fileIO);
+ LockManagers.defaultLockManager(), fileIO, ImmutableMap.of());
Review Comment:
Use this [initialize()](https://github.com/apache/iceberg/blob/b8a796effdca02108b2af005f977cbeb0d9cd718/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java#L160) in the test ensures all fields are initialized. Same to the other related changes in the integration tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #5784: AWS: update AWS Integration Test to fix false positives
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on PR #5784:
URL: https://github.com/apache/iceberg/pull/5784#issuecomment-1256431701
Thanks for fixing the tests!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5784: AWS: update AWS Integration Test to fix false positives
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5784:
URL: https://github.com/apache/iceberg/pull/5784#discussion_r973655588
##########
aws/src/integration/java/org/apache/iceberg/aws/TestDefaultAwsClientFactory.java:
##########
@@ -67,7 +67,7 @@ public void testS3FileIoCredentialsOverride() {
AssertHelpers.assertThrows("Should fail request because of bad access key",
S3Exception.class,
"The AWS Access Key Id you provided does not exist in our records",
- () -> s3Client.getObject(GetObjectRequest.builder().bucket("bucket").key("key").build()));
+ () -> s3Client.getObject(GetObjectRequest.builder().bucket(AwsIntegTestUtil.testBucketName()).key("key").build()));
Review Comment:
If the bucket named "bucket" does not exist on the user's account or not created at the region specified by the user for this test, this test will throw an error message like:
```
The bucket you are attempting to access must be addressed using the specified endpoint.....
```
Hence, I think we should use the bucket specified by the user instead to ensure the desired Exception is caught by the assertion statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5784: AWS: update AWS Integration Test to fix false positives
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5784:
URL: https://github.com/apache/iceberg/pull/5784#discussion_r973655330
##########
aws/src/integration/java/org/apache/iceberg/aws/TestAssumeRoleAwsClientFactory.java:
##########
@@ -75,7 +75,7 @@ public void before() {
assumeRoleProperties = Maps.newHashMap();
assumeRoleProperties.put(AwsProperties.CLIENT_FACTORY, AssumeRoleAwsClientFactory.class.getName());
assumeRoleProperties.put(AwsProperties.HTTP_CLIENT_TYPE, AwsProperties.HTTP_CLIENT_TYPE_APACHE);
- assumeRoleProperties.put(AwsProperties.CLIENT_ASSUME_ROLE_REGION, "us-east-1");
+ assumeRoleProperties.put(AwsProperties.CLIENT_ASSUME_ROLE_REGION, AwsIntegTestUtil.testRegion());
Review Comment:
If the bucket is created in a different region, we need to use the region specified by the user to perform integration tests. Otherwise, we may get a 400 bad request error when sending a request from `us-east-1`
##########
aws/src/integration/java/org/apache/iceberg/aws/TestDefaultAwsClientFactory.java:
##########
@@ -67,7 +67,7 @@ public void testS3FileIoCredentialsOverride() {
AssertHelpers.assertThrows("Should fail request because of bad access key",
S3Exception.class,
"The AWS Access Key Id you provided does not exist in our records",
- () -> s3Client.getObject(GetObjectRequest.builder().bucket("bucket").key("key").build()));
+ () -> s3Client.getObject(GetObjectRequest.builder().bucket(AwsIntegTestUtil.testBucketName()).key("key").build()));
Review Comment:
If the bucket named "bucket" does not exist on the user's account or not created at the region specified by the user for this test, the request will throw an error message like:
```
The bucket you are attempting to access must be addressed using the specified endpoint.....
```
Hence, we need to use the bucket specified by the user instead to ensure the desired Exception is caught by the assertion statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5784: AWS: update AWS Integration Test to fix false positives
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5784:
URL: https://github.com/apache/iceberg/pull/5784#discussion_r973655588
##########
aws/src/integration/java/org/apache/iceberg/aws/TestDefaultAwsClientFactory.java:
##########
@@ -67,7 +67,7 @@ public void testS3FileIoCredentialsOverride() {
AssertHelpers.assertThrows("Should fail request because of bad access key",
S3Exception.class,
"The AWS Access Key Id you provided does not exist in our records",
- () -> s3Client.getObject(GetObjectRequest.builder().bucket("bucket").key("key").build()));
+ () -> s3Client.getObject(GetObjectRequest.builder().bucket(AwsIntegTestUtil.testBucketName()).key("key").build()));
Review Comment:
If the bucket named "bucket" does not exist on the user's account or not created at the region specified by the user for this test, this test will throw an error message like:
```
The bucket you are attempting to access must be addressed using the specified endpoint.....
```
, since the request validation happens before the credentials validation.
Hence, we need to use the bucket specified by the user instead to ensure the desired Exception is caught by the assertion statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5784: AWS: update AWS Integration Test to fix false positives
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5784:
URL: https://github.com/apache/iceberg/pull/5784#discussion_r973655588
##########
aws/src/integration/java/org/apache/iceberg/aws/TestDefaultAwsClientFactory.java:
##########
@@ -67,7 +67,7 @@ public void testS3FileIoCredentialsOverride() {
AssertHelpers.assertThrows("Should fail request because of bad access key",
S3Exception.class,
"The AWS Access Key Id you provided does not exist in our records",
- () -> s3Client.getObject(GetObjectRequest.builder().bucket("bucket").key("key").build()));
+ () -> s3Client.getObject(GetObjectRequest.builder().bucket(AwsIntegTestUtil.testBucketName()).key("key").build()));
Review Comment:
If the bucket named "bucket" does not exist on the user's account or not created at the region specified by the user for this test, this test will throw an error message like:
```
The bucket you are attempting to access must be addressed using the specified endpoint.....
```
Hence, we need to use the bucket specified by the user instead to ensure the desired Exception is caught by the assertion statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org