dimas-b commented on code in PR #724:
URL: https://github.com/apache/polaris/pull/724#discussion_r1922681612
##########
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:
Re: the "common denominator" point, I understand that there may be existing
features in Polaris what allow for wide and unrestricted customizations.
However, I do not think it is a valid reason to justify exposing _new_ areas
for customization without imposing some structure.
For example, currently, storage configuration saved in the internal
properties has to map to `PolarisStorageConfigurationInfo` (and sub-classes).
So, inputs to `FileIOFactory` as "sanitized" by going through those classes.
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.
It is not my intention to block this PR, but I still do not see how my
example (with upgrades) can work conveniently for OSS users in the wild and for
Polaris developers. We can always say that whoever puts custom values into
"internal" properties will be in the "unsupported" upgrade land, but that,
IMHO, it too much of a burden for users. Not all users will want or be able to
or deal with java code internals in Polaris / Iceberg (by extension).
To restate my previous suggestions in more concrete terms, I think it would
be beneficial for the customizations that this PR is expected to enable to be
first-class properties in `PolarisStorageConfigurationInfo` (or sub-classes).
Then, the conversion of internal properties to FileIO properties can be done
by `FileIOFactory` taking a `PolarisStorageConfigurationInfo` as input, or by
Polaris Core (passing a generic `Map` to `FileIOFactory`). From my POV this
concern is secondary if we establish a clear set of what is possible to
configure.
Again, if the expected customizations in the internal properties are not
suitable for general OSS use, I think we ought to introduce another plugin
point (e.g. `FileIOConfigurer`) with the default OSS impl. working the same way
as `refreshIOWithCredentials()`. This way it is simply not possible for OSS
user to enter the unsupported land without manually deploying their own Polaris
extension code.
--
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]
