Skip to content

Conversation

@jackye1995
Copy link
Contributor

@danielcweeks as discussed in #1844 , separated PR for assume role client factory support.

} catch (GlueException e) {
LOG.error("fail to create or delete Glue database", e);
Assert.fail("create and delete namespace should succeed");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should add a finally to retry to drop the namespace.

Assert.assertEquals("Should see 403 error code", 403, e.statusCode());
}

inputFile = s3FileIO.newInputFile("s3://" + AwsIntegTestUtil.testBucketName() + "/allowed/file");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you delete this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file does not exist, I call inputFile.exsits() just to check it has access permission.

@rdblue rdblue requested a review from danielcweeks January 5, 2021 22:10
@rdblue
Copy link
Contributor

rdblue commented Jan 5, 2021

Looks good to me, but I haven't been following much of the config discussion so I'll defer to @danielcweeks.

@danielcweeks
Copy link
Contributor

This looks good to me as well, but I think @giovannifumarola comment might still need to be addressed. Let me know and I'll push it through.

Copy link
Contributor

@danielcweeks danielcweeks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jackye1995!

@danielcweeks danielcweeks merged commit 915a175 into apache:master Jan 6, 2021
XuQianJin-Stars pushed a commit to XuQianJin-Stars/iceberg that referenced this pull request Mar 22, 2021
* AWS: add client factory for assume role use case

* fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants