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 2021/12/03 06:59:31 UTC
[GitHub] [iceberg] jackye1995 opened a new pull request #3659: Aliyun: align property names with AWS module
jackye1995 opened a new pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659
@openinx @xingbowu
I was writing #3658 and I noticed that there is some inconsistency between Aliyun and AWS module. I suggest making the following changes:
1. use `client.access-key-id` and `client.secret-access-key` as property names,
`.` is used to build hierarchy to config name, but `access.key.id` does not have any hierarchy in it. Also I checked Aliyun doc, the name is secret access key, not access key secret, as shown in the builder method:
```
public class OSSClientBuilder implements OSSBuilder {
@Override
public OSS build(String endpoint, String accessKeyId, String secretAccessKey) {
return new OSSClient(endpoint, getDefaultCredentialProvider(accessKeyId, secretAccessKey),
getClientConfiguration());
}
```
I made `client` as the prefix because it seems like your credentials config will affect all the clients. In AWS module, I made the config `s3.access-key-id` because it would only affect `s3`.
2. I renamed `AliyunClientFactories.load` to `AliyunClientFactories.from` to be consistent with `AwsClientFactories.from`. It also seems to me that the default client did not initialize and load the properties, so I also fixed the method and added 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 a change in pull request #3659: Aliyun: align property names with AWS module
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659#discussion_r763472082
##########
File path: aliyun/src/main/java/org/apache/iceberg/aliyun/AliyunProperties.java
##########
@@ -50,7 +50,7 @@
* For more information about how to obtain an AccessKey pair, see:
* https://www.alibabacloud.com/help/doc-detail/53045.htm
*/
- public static final String ACCESS_KEY_SECRET = "access.key.secret";
+ public static final String CLIENT_SECRET_ACCESS_KEY = "client.secret-access-key";
Review comment:
that makes sense, I will update the PR
--
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] openinx commented on a change in pull request #3659: Aliyun: align property names with AWS module
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659#discussion_r762792369
##########
File path: aliyun/src/main/java/org/apache/iceberg/aliyun/AliyunProperties.java
##########
@@ -50,7 +50,7 @@
* For more information about how to obtain an AccessKey pair, see:
* https://www.alibabacloud.com/help/doc-detail/53045.htm
*/
- public static final String ACCESS_KEY_SECRET = "access.key.secret";
+ public static final String CLIENT_SECRET_ACCESS_KEY = "client.secret-access-key";
Review comment:
In alibaba cloud, we often say the access key pair is composed by a `key` and a `secret`. The `key` is similar to the `username`, and the `secret` is similar to the password. You can see the document [1] and [2].
So in my view, a better way is to rename it as `client.access-key-secret` to match the alibaba cloud. About the OSS Java SDK, although they use a different way to name the variable but I think the document is more suitable & readable for most of the aliyun people.
[1]. https://www.alibabacloud.com/help/doc-detail/63482.htm
[2]. https://www.alibabacloud.com/help/doc-detail/53045.htm
--
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] openinx commented on a change in pull request #3659: Aliyun: align property names with AWS module
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659#discussion_r763569099
##########
File path: aliyun/src/main/java/org/apache/iceberg/aliyun/AliyunClientFactories.java
##########
@@ -36,12 +37,10 @@ public static AliyunClientFactory defaultFactory() {
return ALIYUN_CLIENT_FACTORY_DEFAULT;
}
- public static AliyunClientFactory load(Map<String, String> properties) {
- if (properties.containsKey(AliyunProperties.CLIENT_FACTORY)) {
- return load(properties.get(AliyunProperties.CLIENT_FACTORY), properties);
- } else {
- return defaultFactory();
- }
+ public static AliyunClientFactory from(Map<String, String> properties) {
+ String factoryImpl = PropertyUtil.propertyAsString(
+ properties, AliyunProperties.CLIENT_FACTORY, DefaultAliyunClientFactory.class.getName());
Review comment:
I think the previous version has a small bug, because the `DefaultAliyunClientFactory` forget to initialize the aliyun properties in the constructors. This PR unified the default aliyun client factory and customized client factory, it's great to avoid the the properties initialization !
--
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] openinx commented on a change in pull request #3659: Aliyun: align property names with AWS module
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659#discussion_r762792369
##########
File path: aliyun/src/main/java/org/apache/iceberg/aliyun/AliyunProperties.java
##########
@@ -50,7 +50,7 @@
* For more information about how to obtain an AccessKey pair, see:
* https://www.alibabacloud.com/help/doc-detail/53045.htm
*/
- public static final String ACCESS_KEY_SECRET = "access.key.secret";
+ public static final String CLIENT_SECRET_ACCESS_KEY = "client.secret-access-key";
Review comment:
In alibaba cloud, we often say the access key pair is composed by a `key` and a `secret`. The `key` is similar to the `username`, and the `secret` is similar to the password. You can see the document [1] and [2].
So in my view, a better way is to rename it as `client.access-key-secret` to match the alibaba cloud document. About the OSS Java SDK, although they use a different way to name the variable but I think the document is more suitable & readable for most of the aliyun people.
[1]. https://www.alibabacloud.com/help/doc-detail/63482.htm
[2]. https://www.alibabacloud.com/help/doc-detail/53045.htm
--
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 #3659: Aliyun: align property names with AWS module
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659#issuecomment-987333727
> I wish the we can have a more clear document about how to define the names
Thanks, I added #3678 to track 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] openinx merged pull request #3659: Aliyun: align property names with AWS module
Posted by GitBox <gi...@apache.org>.
openinx merged pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659
--
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 #3659: Aliyun: align property names with AWS module
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659#issuecomment-986295621
I would like to see this rename, although I know there is pushback because some people have started using the unreleased feature. I don't like potentially setting a precedent that people using unreleased features can affect code that gets eventually released, but I don't think it matters that much here.
--
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] xingbowu commented on pull request #3659: Aliyun: align property names with AWS module
Posted by GitBox <gi...@apache.org>.
xingbowu commented on pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659#issuecomment-986237320
Hi Jack, Regarding the name style of access key, I agree with you that the best way is to line up them between aws and aliyun module。however, considering legacy reason for user behavior, we decided to keep it as it is.
could you check the discussion comments in PR3553 https://github.com/apache/iceberg/pull/3553#discussion_r752046575
feel free to let me know your further idea?
it looks good to me to rename `AliyunClientFactories.load`
--
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] openinx commented on pull request #3659: Aliyun: align property names with AWS module
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659#issuecomment-986545988
I can accept the refactor names if others really don't like the original nested names which does not match the apache iceberg name styles (I wish the we can have a more clear document about how to define the names, then maybe I could avoid this name confusion when I developed the original version.).
--
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 #3659: Aliyun: align property names with AWS module
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3659:
URL: https://github.com/apache/iceberg/pull/3659#issuecomment-986489371
@openinx any thoughts?
--
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