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/06/15 08:06:54 UTC
[GitHub] [iceberg] dungdm93 opened a new pull request, #5046: AWS: support configure region
dungdm93 opened a new pull request, #5046:
URL: https://github.com/apache/iceberg/pull/5046
Currently, to configure AWS region, we need to configure at global scope such as in environment variable `AWS_REGION=us-east-1`.
In the case Spark (or Flink) have multiple catalogs, each use different bucket in different region, there is no way to do it now. So this PR is aim to allow us to specify region for each catalog.
--
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] dungdm93 commented on a diff in pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r912585501
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -280,6 +280,12 @@ public class AwsProperties implements Serializable {
*/
public static final String CLIENT_ASSUME_ROLE_REGION = "client.assume-role.region";
+ /**
+ * The value must be one of {@link software.amazon.awssdk.regions.Region}, such as 'us-east-1'.
+ * For more details, see https://docs.aws.amazon.com/general/latest/gr/rande.html
+ */
+ public static final String CLIENT_REGION = "client.region";
Review Comment:
@rdblue , @amogh-jahagirdar I'm ok with `client`.
But I wonder is there any cases `s3`/`glue`/`dymano` in a catalog are located in different region.
So, in that case, we need different key for each service like `s3.region`, `glue.region` and `dymano.region`.
--
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] rdblue commented on a diff in pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r910172137
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -99,29 +102,40 @@ static class DefaultAwsClientFactory implements AwsClientFactory {
public S3Client s3() {
return S3Client.builder()
.httpClientBuilder(configureHttpClientBuilder(httpClientType))
- .applyMutation(builder -> configureEndpoint(builder, s3Endpoint))
+ .applyMutation(builder -> configureRegion(builder, s3Endpoint))
+ .applyMutation(builder -> configureEndpoint(builder, region))
Review Comment:
This looks suspicious. It calls `configureRegion` with `s3Endpoint` and `configureEndpoint` with `region`.
--
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] dungdm93 commented on pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#issuecomment-1186482295
@[amogh-jahagirdar Currently, I'm see the faction of configurations between services, e.g: `glue`, `kms`, `dynamo` missing the way to set `credentialsProvider`
WDYT about having common configuration of `endpoint`, `region`, `access-key-id`, `secret-access-key` and `session-token` for all services.
```java
String S3FILEIO_PREFIX = "s3."
String GLUE_PREFIX = "glue."
/**
* To set S3 region, use s3.region, to set Glue region set glue.region, etc...
**/
String REGION_CONFIG = "region"
/**
* To set S3 endpoint, use s3.endpoint, to set Glue region set glue.endpoint, etc...
**/
String ENDPOINT_CONFIG = "endpoint"
...
```
--
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] amogh-jahagirdar commented on a diff in pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r911184125
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -99,29 +102,40 @@ static class DefaultAwsClientFactory implements AwsClientFactory {
public S3Client s3() {
return S3Client.builder()
.httpClientBuilder(configureHttpClientBuilder(httpClientType))
- .applyMutation(builder -> configureEndpoint(builder, s3Endpoint))
+ .applyMutation(builder -> configureRegion(builder, s3Endpoint))
+ .applyMutation(builder -> configureEndpoint(builder, region))
.serviceConfiguration(s3Configuration(s3PathStyleAccess, s3UseArnRegionEnabled))
.credentialsProvider(credentialsProvider(s3AccessKeyId, s3SecretAccessKey, s3SessionToken))
.build();
}
@Override
public GlueClient glue() {
- return GlueClient.builder().httpClientBuilder(configureHttpClientBuilder(httpClientType)).build();
+ return GlueClient.builder()
+ .httpClientBuilder(configureHttpClientBuilder(httpClientType))
+ .applyMutation(builder -> configureEndpoint(builder, region))
Review Comment:
I don't think this is right, we're calling configureEndpoint with the region, instead of the new configureRegion?
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -99,29 +102,40 @@ static class DefaultAwsClientFactory implements AwsClientFactory {
public S3Client s3() {
return S3Client.builder()
.httpClientBuilder(configureHttpClientBuilder(httpClientType))
- .applyMutation(builder -> configureEndpoint(builder, s3Endpoint))
+ .applyMutation(builder -> configureRegion(builder, s3Endpoint))
+ .applyMutation(builder -> configureEndpoint(builder, region))
.serviceConfiguration(s3Configuration(s3PathStyleAccess, s3UseArnRegionEnabled))
.credentialsProvider(credentialsProvider(s3AccessKeyId, s3SecretAccessKey, s3SessionToken))
.build();
}
@Override
public GlueClient glue() {
- return GlueClient.builder().httpClientBuilder(configureHttpClientBuilder(httpClientType)).build();
+ return GlueClient.builder()
+ .httpClientBuilder(configureHttpClientBuilder(httpClientType))
+ .applyMutation(builder -> configureEndpoint(builder, region))
+ .build();
}
@Override
public KmsClient kms() {
- return KmsClient.builder().httpClientBuilder(configureHttpClientBuilder(httpClientType)).build();
+ return KmsClient.builder()
+ .httpClientBuilder(configureHttpClientBuilder(httpClientType))
+ .applyMutation(builder -> configureEndpoint(builder, region))
+ .build();
}
@Override
public DynamoDbClient dynamo() {
- return DynamoDbClient.builder().httpClientBuilder(configureHttpClientBuilder(httpClientType)).build();
+ return DynamoDbClient.builder()
+ .httpClientBuilder(configureHttpClientBuilder(httpClientType))
+ .applyMutation(builder -> configureEndpoint(builder, region))
Review Comment:
same
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -280,6 +280,12 @@ public class AwsProperties implements Serializable {
*/
public static final String CLIENT_ASSUME_ROLE_REGION = "client.assume-role.region";
+ /**
+ * The value must be one of {@link software.amazon.awssdk.regions.Region}, such as 'us-east-1'.
+ * For more details, see https://docs.aws.amazon.com/general/latest/gr/rande.html
+ */
+ public static final String CLIENT_REGION = "client.region";
Review Comment:
I'm guessing it's just following the existing namespace convention for AWS clients; unless I'm missing something, I don't think for region it needs to be qualified in the "client" namespace, so just `region` would suffice.
--
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] amogh-jahagirdar commented on a diff in pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r922590682
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -140,17 +155,22 @@ public void initialize(Map<String, String> properties) {
AwsProperties.S3_USE_ARN_REGION_ENABLED,
AwsProperties.S3_USE_ARN_REGION_ENABLED_DEFAULT
);
-
ValidationException.check(
(s3AccessKeyId == null) == (s3SecretAccessKey == null),
- "S3 client access key ID and secret access key must be set at the same time");
+ "S3 client access key ID and secret access key must be set at the same time"
+ );
+
+ this.kmsRegion = properties.get(AwsProperties.KMS_REGION);
+ this.glueRegion = properties.get(AwsProperties.GLUE_REGION);
+ this.dynamoDbRegion = properties.get(AwsProperties.DYNAMODB_REGION);
this.dynamoDbEndpoint = properties.get(AwsProperties.DYNAMODB_ENDPOINT);
+
Review Comment:
Nit: unnecessary new line
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -539,6 +563,7 @@ public void setGlueCatalogId(String id) {
public boolean glueCatalogSkipArchive() {
return glueCatalogSkipArchive;
}
+
Review Comment:
Nit: Unnecessary new line
--
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] dungdm93 commented on a diff in pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r913339098
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -280,6 +280,12 @@ public class AwsProperties implements Serializable {
*/
public static final String CLIENT_ASSUME_ROLE_REGION = "client.assume-role.region";
+ /**
+ * The value must be one of {@link software.amazon.awssdk.regions.Region}, such as 'us-east-1'.
+ * For more details, see https://docs.aws.amazon.com/general/latest/gr/rande.html
+ */
+ public static final String CLIENT_REGION = "client.region";
Review Comment:
I decided to create a key for each service: `s3.region`, `glue.region`, `dymano.region` and `kms.region`.
--
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] dungdm93 commented on a diff in pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r912581797
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -99,29 +102,40 @@ static class DefaultAwsClientFactory implements AwsClientFactory {
public S3Client s3() {
return S3Client.builder()
.httpClientBuilder(configureHttpClientBuilder(httpClientType))
- .applyMutation(builder -> configureEndpoint(builder, s3Endpoint))
+ .applyMutation(builder -> configureRegion(builder, s3Endpoint))
+ .applyMutation(builder -> configureEndpoint(builder, region))
Review Comment:
Sorry, my bad.
--
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] rdblue commented on a diff in pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r910172790
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -280,6 +280,12 @@ public class AwsProperties implements Serializable {
*/
public static final String CLIENT_ASSUME_ROLE_REGION = "client.assume-role.region";
+ /**
+ * The value must be one of {@link software.amazon.awssdk.regions.Region}, such as 'us-east-1'.
+ * For more details, see https://docs.aws.amazon.com/general/latest/gr/rande.html
+ */
+ public static final String CLIENT_REGION = "client.region";
Review Comment:
Why `client.region` and not just `region`?
--
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] ChristinaTech commented on pull request #5046: AWS: support configure region
Posted by "ChristinaTech (via GitHub)" <gi...@apache.org>.
ChristinaTech commented on PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#issuecomment-1546775959
This has the same goal as a PR I was going to open @dungdm93. Do you plan to continue or shall I pick up from where you left things?
--
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] dungdm93 commented on a diff in pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r922812146
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -140,17 +155,22 @@ public void initialize(Map<String, String> properties) {
AwsProperties.S3_USE_ARN_REGION_ENABLED,
AwsProperties.S3_USE_ARN_REGION_ENABLED_DEFAULT
);
-
ValidationException.check(
(s3AccessKeyId == null) == (s3SecretAccessKey == null),
- "S3 client access key ID and secret access key must be set at the same time");
+ "S3 client access key ID and secret access key must be set at the same time"
+ );
+
+ this.kmsRegion = properties.get(AwsProperties.KMS_REGION);
+ this.glueRegion = properties.get(AwsProperties.GLUE_REGION);
+ this.dynamoDbRegion = properties.get(AwsProperties.DYNAMODB_REGION);
this.dynamoDbEndpoint = properties.get(AwsProperties.DYNAMODB_ENDPOINT);
+
Review Comment:
Removed.
--
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] dungdm93 commented on a diff in pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r922811804
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -539,6 +563,7 @@ public void setGlueCatalogId(String id) {
public boolean glueCatalogSkipArchive() {
return glueCatalogSkipArchive;
}
+
Review Comment:
It's done by IntelliJ formatter. I think the previous commit forgot this one.
--
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] rdblue commented on pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#issuecomment-1170203224
@jackye1995, can you take a look at this?
--
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] dungdm93 commented on a diff in pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on code in PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#discussion_r912583883
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -99,29 +102,40 @@ static class DefaultAwsClientFactory implements AwsClientFactory {
public S3Client s3() {
return S3Client.builder()
.httpClientBuilder(configureHttpClientBuilder(httpClientType))
- .applyMutation(builder -> configureEndpoint(builder, s3Endpoint))
+ .applyMutation(builder -> configureRegion(builder, s3Endpoint))
+ .applyMutation(builder -> configureEndpoint(builder, region))
.serviceConfiguration(s3Configuration(s3PathStyleAccess, s3UseArnRegionEnabled))
.credentialsProvider(credentialsProvider(s3AccessKeyId, s3SecretAccessKey, s3SessionToken))
.build();
}
@Override
public GlueClient glue() {
- return GlueClient.builder().httpClientBuilder(configureHttpClientBuilder(httpClientType)).build();
+ return GlueClient.builder()
+ .httpClientBuilder(configureHttpClientBuilder(httpClientType))
+ .applyMutation(builder -> configureEndpoint(builder, region))
Review Comment:
Yeah. It's my bad. fixed.
--
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 #5046: AWS: support configure region
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#issuecomment-1464284121
Hi sorry I overlooked this PR for a long time, do you still plan to finish this feature?
--
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] nastra commented on pull request #5046: AWS: support configure region
Posted by GitBox <gi...@apache.org>.
nastra commented on PR #5046:
URL: https://github.com/apache/iceberg/pull/5046#issuecomment-1271707007
fixes https://github.com/apache/iceberg/issues/5769
--
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