-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[feat](catalog)AWS Glue supports S3 access via IAM AssumeRole. #56311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 1520 ms |
TPC-DS: Total hot run time: 2751 ms |
ClickBench: Total hot run time: 0.08 s |
FE Regression Coverage ReportIncrement line coverage |
...rc/main/java/com/amazonaws/glue/catalog/credentials/ConfigurationAWSCredentialsProvider.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/amazonaws/glue/catalog/credentials/ConfigurationAWSCredentialsProvider.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/datasource/property/ParamRules.java
Show resolved
Hide resolved
| hiveConf.set(AWS_CATALOG_CREDENTIALS_PROVIDER_FACTORY_CLASS_KEY, | ||
| "com.amazonaws.glue.catalog.credentials.ConfigurationAWSCredentialsProviderFactory"); | ||
| hiveConf.set("hive.metastore.type", "glue"); | ||
| setHiveConfPropertiesIfNotNull(hiveConf, AWSGlueConfig.AWS_GLUE_ACCESS_KEY, baseProperties.glueAccessKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to check if ak sk is set or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the authentication parameters are already validated during the initialization phase, we only need to perform null checks during usage.
| props.put("client.credentials-provider.glue.session_token", glueProperties.glueSessionToken); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest:
if (ak, sk) {
} else if (iam role) {
} else {
throw exception
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s just a personal preference—readability is much better this way, and deeply nested if/else statements aren’t very readable.
|
run buildall |
ClickBench: Total hot run time: 30.39 s |
FE Regression Coverage ReportIncrement line coverage |
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
ClickBench: Total hot run time: 30.69 s |
FE Regression Coverage ReportIncrement line coverage |
morningman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
PR approved by at least one committer and no changes requested. |
### What problem does this PR solve? ``` CREATE CATALOG `hive_glue_iam_role` PROPERTIES ( "type" = "hms", "hive.metastore.type" = "glue", "glue.role_arn" = "arn:aws:iam::8888888888:role/christen", "glue.endpoint" = "https://glue.us-east-1.amazonaws.com" ); CREATE CATALOG `iceberg_glue_iam_role` PROPERTIES ( "type" = "iceberg", "iceberg.catalog.type" = "glue", "warehouse" = "s3://bucket/regression/glue/", "glue.role_arn" = "arn:aws:iam::8888888888:role/christen", "glue.endpoint" = "https://glue.us-east-1.amazonaws.com" ); ```
…e#56311) ### What problem does this PR solve? ``` CREATE CATALOG `hive_glue_iam_role` PROPERTIES ( "type" = "hms", "hive.metastore.type" = "glue", "glue.role_arn" = "arn:aws:iam::8888888888:role/christen", "glue.endpoint" = "https://glue.us-east-1.amazonaws.com" ); CREATE CATALOG `iceberg_glue_iam_role` PROPERTIES ( "type" = "iceberg", "iceberg.catalog.type" = "glue", "warehouse" = "s3://bucket/regression/glue/", "glue.role_arn" = "arn:aws:iam::8888888888:role/christen", "glue.endpoint" = "https://glue.us-east-1.amazonaws.com" ); ``` (cherry picked from commit 7202586)
What problem does this PR solve?
apache/doris-website#2912