dennishuo commented on code in PR #724:
URL: https://github.com/apache/polaris/pull/724#discussion_r1919418953
##########
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:
Another way to look at this is to consider the fact that
`BasePolarisCatalog::refreshIOWithCredentials` is already trying to serve as a
consolidated factory method for a FileIO. And the method signature here
indicates that we think the following information is all pertinent to the
construction of a FileIO:
TableIdentifier identifier,
Set<String> readLocations,
PolarisResolvedPathWrapper resolvedStorageEntity,
Map<String, String> tableProperties,
Set<PolarisStorageActions> storageActions
We only later added a FileIOFactory, because some custom users of Polaris
didn't want to fork the code and modify the body of
`BasePolarisCatalog::refreshIOWithCredentials`. So in a sense the purpose of
`FileIOFactory` is to say a custom Polaris deployment is capable of fully
customizing the way a FileIO is constructed, possibly with layers of e.g.
throttling, auditing, whatever without ever needing to "fork" Polaris code, but
only needing to configure an injected `FileIOFactory` class.
In this sense, there's an argument here for plumbing *all* those arguments
through to the `FileIOFactory`.
If the factory wants to emit an alert whenever `readLocations` contains the
substring `secret-passwords`, then it can easily do that. If the factory wants
to redirect all requests that have `WRITE` actions through a special auditing
proxy, it can do that too. If it wants to look at the `entityVersion` and emit
special logs whenever the entityVersion is divisible by 42, it can also do that.
--
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]
