adutra commented on code in PR #1572: URL: https://github.com/apache/polaris/pull/1572#discussion_r2085624428
########## helm/polaris/README.md: ########## @@ -220,7 +220,7 @@ kubectl delete namespace polaris | extraVolumeMounts | list | `[]` | Extra volume mounts to add to the polaris container. See https://kubernetes.io/docs/concepts/storage/volumes/. | | extraVolumes | list | `[]` | Extra volumes to add to the polaris pod. See https://kubernetes.io/docs/concepts/storage/volumes/. | | features | object | `{"defaults":{},"realmOverrides":{}}` | Polaris features configuration. | -| features.defaults | object | `{}` | Features to enable or disable globally. If a feature is not present in the map, the default built-in value is used. | +| features | object | `{}` | Features to enable or disable globally. If a feature is not present in the map, the default built-in value is used. | Review Comment: FYI this file should not be edited directly. Instead, you should run: ```shell helm-docs --chart-search-root=helm ``` And, if necessary, modify `README.md.gotmpl`, and finally, commit the changes. I just tried it on your branch, and unfortunately, it seems the file is already out of sync with `README.md.gotmpl` 😞 ########## quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusFeaturesConfiguration.java: ########## @@ -19,15 +19,23 @@ package org.apache.polaris.service.quarkus.config; import io.smallrye.config.ConfigMapping; +import io.smallrye.config.WithParentName; import java.util.Map; import org.apache.polaris.service.config.FeaturesConfiguration; @ConfigMapping(prefix = "polaris.features") public interface QuarkusFeaturesConfiguration extends FeaturesConfiguration { + @WithParentName @Override Map<String, String> defaults(); Review Comment: FYI this is creating an unwanted behavior: I injected `FeaturesConfiguration` in `DefaultConfigurationStoreTest` and here are the contents of `featuresConfiguration.defaults()`: ``` realm-overrides."realm1"."ALLOW_SPECIFYING_FILE_IO_IMPL"=false ALLOW_SPECIFYING_FILE_IO_IMPL=true ENABLE_GENERIC_TABLES=false ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING=false SUPPORTED_CATALOG_CONNECTION_TYPES=["ICEBERG_REST"] SUPPORTED_CATALOG_STORAGE_TYPES=["S3","GCS","AZURE","FILE"] ``` You will notice the strange `realm-overrides."realm1"."ALLOW_SPECIFYING_FILE_IO_IMPL=false` entry. The contents of `featuresConfiguration.realmOverrides()` look correct though: ``` realm1={ ALLOW_SPECIFYING_FILE_IO_IMPL=false } ``` At this point, I wonder if a better solution wouldn't be to simply define one single map here to hold all the configs: ```java public interface QuarkusFeaturesConfiguration extends FeaturesConfiguration { @WithParentName @Override Map<String, String> allConfigs(); } ``` And parse that map manually (we are already doing it half-manually anyway): ```java public interface FeaturesConfiguration { Map<String, String> allConfigs(); default Map<String, Object> parseDefaults(ObjectMapper objectMapper) { Map<String, Object> defaults = new HashMap<>(); for (String key : allConfigs().keySet()) { if (!key.contains(".")) { defaults.put(key, convertProperty(objectMapper, key, allConfigs().get(key))); } } return defaults; } default Map<String, Map<String, Object>> parseRealmOverrides(ObjectMapper objectMapper) { Map<String, Map<String, Object>> realmOverrides = new HashMap<>(); for (String key : allConfigs().keySet()) { if (key.contains(".")) { String[] parts = key.split("\\."); String realm = parts[0]; String configName = parts[1]; Map<String, Object> realmValues = realmOverrides.computeIfAbsent(realm, k -> new HashMap<>()); realmValues.put(configName, convertProperty(objectMapper, key, allConfigs().get(key))); } } return realmOverrides; } private static Object convertProperty(ObjectMapper objectMapper, String key, String value) { try { JsonNode node = objectMapper.readTree(value); return configValue(node); } catch (JsonProcessingException e) { throw new RuntimeException( "Invalid JSON value for feature configuration: " + key, e); } } private static Object configValue(JsonNode node) { // same as before } } ``` WDYT? (this obviously assumes that a feature configuration name cannot contain a dot.) -- 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]
