XJDKC commented on code in PR #724:
URL: https://github.com/apache/polaris/pull/724#discussion_r1922758284
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -1627,6 +1627,12 @@ private FileIO refreshIOWithCredentials(
// the credentials should always override table-level properties, since
// storage configuration will be found at whatever entity defines it
tableProperties.putAll(credentialsMap);
+
+ // Populate the internal properties to FileIO in case the FileIO
implementation needs them
+ Map<String, String> internalProperties =
+
storageInfoEntity.map(PolarisEntity::getInternalPropertiesAsMap).orElse(Map.of());
Review Comment:
> With this PR, it will be possible for admin users to configure storage
access parameters that are not reflected in Polaris code, and as such are
obscure from the Polaris developers' POV.
Why would OSS Polaris developers need to know the custom properties
configured by "Customized" Polaris users? These properties are meant for
user-specific customization and should not require visibility or concern from
OSS developers unless they directly impact OSS functionality.
> I still do not see how my example (with upgrades) can work conveniently
for OSS users in the wild and for Polaris developers.
For OSS users, the logic of refreshIOWithCredentials is moved to
DefaultFileIOFactory. If users do not customize the FileIO, they can upgrade
directly without issues.
For "Customized" Polaris users, if they customize Polaris by injecting
properties into internalProperties, they can still upgrade directly unless
internalProperties is completely removed.
Additionally, since all information available to `refreshIOWithCredentials`
is passed to `FileIOFactory` (including resolved entities), custom properties
can be stored elsewhere in the entity object. This is one of the reasons why we
avoid passing `internalProperties` directly to `FileIOFactory` and instead pass
all arguments from `refreshIOWithCredentials`.
> This PR is expected to enable to be first-class properties in
PolarisStorageConfigurationInfo
How can a custom `PolarisMetastoreManager` inject a property not defined in
the OSS `PolarisStorageConfigurationInfo` as the first class property?
> Introduce another plugin point (e.g. FileIOConfigurer)
```java
public class FileIOConfig {
private final String impl;
private final Map<String, String> properties;
}
public interface FileIOConfigurer {
FileIOConfig generateConfig(
String impl,
TableIdentifier identifier,
Set<String> readLocations,
PolarisResolvedPathWrapper resolvedStorageEntity,
Map<String, String> tableProperties,
Set<PolarisStorageActions> storageActions);
}
public void refreshIOWithCredentials(...) {
// ......
FileIOConfigurer fileIOConfigurer = new DefaultFileIOConfiguer(...);
FileIOConfig fileIOConfig = fileIOConfigurer.generateConfig();
FileIO fileIO = loadFileIO(FileIOConfig);
// .....
return fileIO;
}
public class FileIOFactory {
FileIO loadFileIO(FileIOConfig) {
String proxyEndpoint = internalProperties.get(PROXY_ENDPOINT);
properties.add(S3FileIOProperties.ENDPOINT, proxyEndpoint); //
translation logic
// Only pass properties to FileIO
return CatalogUtil.loadFileIO(impl, properties, new Configuration());
}
}
```
This is the proposed solution? From my POV, this solution is almost the
same as the the new `FileIOFactory` proposal but we split the logic into two
classes, but I'm okay with both.
--
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]
