Skip to content

Conversation

@jackye1995
Copy link
Contributor

@jackye1995 jackye1995 commented Dec 3, 2021

@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 names, 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.

  1. 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.

@github-actions github-actions bot added the ALIYUN label Dec 3, 2021
@jackye1995 jackye1995 requested a review from openinx December 3, 2021 07:00
@jackye1995 jackye1995 added this to the Iceberg 0.13.0 Release milestone Dec 3, 2021
@xingbowu
Copy link
Contributor

xingbowu commented Dec 5, 2021

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 #3553 (comment)
feel free to let me know your further idea?

it looks good to me to rename AliyunClientFactories.load

@rdblue
Copy link
Contributor

rdblue commented Dec 5, 2021

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.

@jackye1995
Copy link
Contributor Author

@openinx any thoughts?

@openinx
Copy link
Member

openinx commented Dec 6, 2021

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.).

* 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";
Copy link
Member

@openinx openinx Dec 6, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense, I will update the PR

@jackye1995
Copy link
Contributor Author

I wish the we can have a more clear document about how to define the names

Thanks, I added #3678 to track this

@jackye1995 jackye1995 requested a review from openinx December 6, 2021 23:07
}
public static AliyunClientFactory from(Map<String, String> properties) {
String factoryImpl = PropertyUtil.propertyAsString(
properties, AliyunProperties.CLIENT_FACTORY, DefaultAliyunClientFactory.class.getName());
Copy link
Member

Choose a reason for hiding this comment

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

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 !

Copy link
Member

@openinx openinx left a comment

Choose a reason for hiding this comment

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

LGTM overall !

@openinx openinx merged commit 65931f5 into apache:master Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants