XJDKC commented on code in PR #701:
URL: https://github.com/apache/polaris/pull/701#discussion_r1911782091
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialProperty.java:
##########
@@ -41,7 +41,12 @@ public enum PolarisCredentialProperty {
"the azure storage account host",
"the azure account name + endpoint that will append to the
ADLS_SAS_TOKEN_PREFIX"),
EXPIRATION_TIME(
- Long.class, "expiration-time", "the expiration time for the access
token, in milliseconds");
+ Long.class, "expiration-time", "the expiration time for the access
token, in milliseconds"),
+ ADDITIONAL_STORAGE_CONFIG(
+ String.class,
+ "additional-storage-config",
+ "the additional storage config for the cloud storage"),
Review Comment:
We don't want to expose these additional properties to the iceberg client,
this is for BasePolarisCatalog to access the storage (`doCommit`, `doRefresh`).
The `PolarisMetaStoreManager` we implements will provide some additional
storage config for custom FileIO to access the storage.
[BasePolarisCatalog.java#L1607-L1631](https://github.com/apache/polaris/blob/7d53b73ca21c93c474c19722356eaac121051fc1/service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java#L1607-L1631)
But thanks for pointing this out, it seems that these properties will be
also passed to the client. Maybe we should filter this property when we return
the creds in the `loadTable` response.
For combining the additional properties with creds, the
`getSubscopedCredentials` api actually currently represents a request for a
dynamic connection-configuration, which happens to mostly be credentials right
now. We should treat it as an api that we can use to get storage config so the
Polaris's FileIO can use these info to access the storage.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
