dimas-b commented on code in PR #1068:
URL: https://github.com/apache/polaris/pull/1068#discussion_r1990512842
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpStorageConfigurationInfo.java:
##########
@@ -44,7 +40,9 @@ public GcpStorageConfigurationInfo(
@JsonProperty(value = "allowedLocations", required = true) @Nonnull
List<String> allowedLocations) {
super(StorageType.GCS, allowedLocations);
- validateMaxAllowedLocations(MAX_ALLOWED_LOCATIONS);
+ validateMaxAllowedLocations(
+ PolarisConfigurationStore.getConfiguration(
+ PolarisConfiguration.STORAGE_CONFIGURATION_MAX_LOCATIONS));
Review Comment:
In this context, my concern is not about how configuration works or how
configuration is described. My concern is with runtime behaviour in case the
new config settings make storage config validation fail.
Whether the admin user had enough warning about risks is irrelevant, IMHO,
because the storage config in question is present in the database. It was
stored some time ago under different rules. I believe Polaris has to honour its
persisted config more than transient validation settings, because Polaris'
primary function as a catalog is to manifest its persisted data to clients in
order for clients to gain access to tables. In this case the validation flags
are not necessary to ensure correctness of operation. They are rather arbitrary
limits from the perspective of accessing tables.
I can foresee how the new flag can cause a Polaris Server to fail to provide
access to tables that used to be accessible before setting the flag. However, I
do not see how this code change can help Polaris Server in dealing with
configuration that have too many locations (for example).
If the concern is that having too many locations is detrimental to
performance, then the user will have to adjust configuration somehow, but again
in that case, what is the value of failing to make unadjusted storage config
available for processing? It can cause hard failures compared to hypothetical
performance problems... unless I'm missing something :)
--
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]
