dennishuo commented on code in PR #724:
URL: https://github.com/apache/polaris/pull/724#discussion_r1917570035
##########
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:
The intent of `internalProperties` really is to have room for some
Polaris-internal persistence items that don't *directly* interface with any
kind of external code, but rather are contained by some Polaris-specific
interpretation layer. So, I'm not sure an allowlist is enough because it still
mixes the domain of config keys that show up in `internalProperties` with the
domain of config keys that happen to be Iceberg-recognizable properties.
Indeed, some internalProperties are translated into "top-level" structured
fields in REST objects, others only control internal behaviors, etc.
So, my thinking here is we should probably be explicit about what we're
trying to do. Specifically, we are trying to give *additional* internal
contextual information to a `FileIOFactory.loadFileIO` call (a Polaris
construct) *not* necessarily to an underlying `FileIO` (an Iceberg core
construct). We might already be improperly mixing too much into just the
open-ended properties map we give to the factory, but IMO we should update the
`FileIOFactory` interface to:
FileIO loadFileIO(String impl, Map<String, String> properties,
PolarisResolvedPathWrapper targetEntity);
Maybe in the future even explicitly passing in some form of call context
FileIO loadFileIO(String impl, Map<String, String> properties,
PolarisResolvedPathWrapper targetEntity, CallContext callContext);
The use case here is for better injectable functionality in the form of the
`FileIOFactory` for Polaris service-owners where custom logic that might relate
to custom internalProperties can easily intercept the construction of new
FileIO instances with all the necessary information about the current request
available.
--
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]
