dimas-b commented on code in PR #724:
URL: https://github.com/apache/polaris/pull/724#discussion_r1920668019
##########
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:
To clarify my concerns from previous message, let's consider this use case:
1. An OSS (admin) user deploys Polaris version 1.0.
2. The user configures access to storage. This step involved defining some
Iceberg FileIO properties, naturally.
3. There's a need/request to refactor Polaris internals in v. 2.0
4. The user upgrades Polaris to 2.0
5. How do we make sure the user's configuration still works? Note that we do
not know what the user actually configured.
IMHO, at step 5 we are bound by having to support Iceberg FileIO
implementations and deal with changes in their behaviour. My point is that in
order for Polaris to be able carry out this duty effectively, the configuration
has to be explicit. That is to say, people evolving the Polaris code have to
know exactly what properties are configurable by end users. For example, with
an generic property bag approach, it will be very hard to use non-Iceberg
FileIO in Polaris 2.0 (for example) even for its _interrnal_ access to storage.
##########
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:
To clarify my concerns from previous messages, let's consider this use case:
1. An OSS (admin) user deploys Polaris version 1.0.
2. The user configures access to storage. This step involved defining some
Iceberg FileIO properties, naturally.
3. There's a need/request to refactor Polaris internals in v. 2.0
4. The user upgrades Polaris to 2.0
5. How do we make sure the user's configuration still works? Note that we do
not know what the user actually configured.
IMHO, at step 5 we are bound by having to support Iceberg FileIO
implementations and deal with changes in their behaviour. My point is that in
order for Polaris to be able carry out this duty effectively, the configuration
has to be explicit. That is to say, people evolving the Polaris code have to
know exactly what properties are configurable by end users. For example, with
an generic property bag approach, it will be very hard to use non-Iceberg
FileIO in Polaris 2.0 (for example) even for its _interrnal_ access to 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]
