Rename SUPPORTED_CATALOG_CONNECTION_TYPES #1959
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |||||||||||||||||||||||||||||||||||||||||
| package org.apache.polaris.core.config; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| import java.util.Optional; | ||||||||||||||||||||||||||||||||||||||||||
| import java.util.Set; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||
| * Internal configuration flags for non-feature behavior changes in Polaris. These flags control | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -34,11 +35,12 @@ public class BehaviorChangeConfiguration<T> extends PolarisConfiguration<T> { | |||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| protected BehaviorChangeConfiguration( | ||||||||||||||||||||||||||||||||||||||||||
| String key, | ||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Why can you have multiple
Contributor
Author
There was a problem hiding this comment. Here is my understanding: A PolarisConfiguration or a configuration for any quarkus app has a Please let me know if I am missing something here.
Contributor
There was a problem hiding this comment. It just seems odd that we would support multiple invalid keys but not multiple valid keys. In my original comment here I wrote:
rather than I do agree that the situation where you'd have multiple valid configs seems contrived, but so does the situation where you'd have multiple invalid configs or really even one invalid config. So if we're overhauling the whole key system, I think it makes sense to go all the way.
Contributor
Author
There was a problem hiding this comment. I don't think we support multiple invalid keys. We track multiple From a semantic perspective, a What we have with the
Contributor
There was a problem hiding this comment. The key is not a primary key though. There's no RDBMS or KV store storing these; they describe entries in a config file. It seems reasonable to expect that multiple different entries could be used to configure the same feature.
Contributor
There was a problem hiding this comment. Consider, too, that the current code needs to deal with a situation where the key has value
Contributor
Author
There was a problem hiding this comment. I disagree with this:
You can have two configurations that store the same logical value and then let the specific module decide upon how to reconcile any conflicting values. But the config service should not register two configurations for the same feature. Regarding the case where the
Contributor
There was a problem hiding this comment.
Your current PR has this functionality... except only partially:
What is the rationale behind this asymmetry? If, as stated before, it is because the key is thought of as a primary key then this is just not correct. You can write a valid config that violates the "primary key constraint" by duplicating an entry.
Contributor
Author
There was a problem hiding this comment. The case with 0 keys is not possible. We can't define a PolarisConfig without a key. That was always the case. Unless you'd prefer if we annotate the field with
The I think we're looking at two entirely different aspects of a configuration.
The two are orthogonal to each other. We should impose constraints on how PolarisConfigs are declared. We can't however impose constraints on how the user defines these configs. If the user does have multiple definitions of the same config, then we apply the one with the highest priority and discard the rest. If indeed we should disallow multiple definitions, then any config that has a default value defined should essentially disallow any overwrites in application.properties. AFAIK, Quarkus doesn't do that and there isn't a way for the app to determine such a condition. |
||||||||||||||||||||||||||||||||||||||||||
| Set<String> legacyKeys, | ||||||||||||||||||||||||||||||||||||||||||
| String description, | ||||||||||||||||||||||||||||||||||||||||||
| T defaultValue, | ||||||||||||||||||||||||||||||||||||||||||
| Optional<String> catalogConfig, | ||||||||||||||||||||||||||||||||||||||||||
| Optional<String> catalogConfigUnsafe) { | ||||||||||||||||||||||||||||||||||||||||||
| super(key, legacyKeys, description, defaultValue, catalogConfig, catalogConfigUnsafe); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| public static final BehaviorChangeConfiguration<Boolean> VALIDATE_VIEW_LOCATION_OVERLAP = | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.polaris.core.config; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class PolarisConfigurationTest { | ||
|
|
||
| @Test | ||
| void testFeatureConfigurationWithLegacyKey() { | ||
| FeatureConfiguration<Boolean> config = | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("FEATURE_CONFIG_WITH_LEGACY_KEY") | ||
| .legacyKey("OLD_FEATURE_CONFIG_WITH_LEGACY_KEY") | ||
| .description("Test configuration with deprecated key") | ||
| .defaultValue(true) | ||
| .buildFeatureConfiguration(); | ||
|
|
||
| assertThat(config.hasLegacyKeys()).isTrue(); | ||
| assertThat(config.legacyKeys()).isEqualTo("OLD_FEATURE_CONFIG_WITH_LEGACY_KEY"); | ||
| assertThat(config.key).isEqualTo("FEATURE_CONFIG_WITH_LEGACY_KEY"); | ||
| assertThat(config.description).isEqualTo("Test configuration with deprecated key"); | ||
| assertThat(config.defaultValue).isTrue(); | ||
| } | ||
|
|
||
| @Test | ||
| void testBehaviorChangeConfigurationWithLegacyKey() { | ||
| BehaviorChangeConfiguration<String> config = | ||
| PolarisConfiguration.<String>builder() | ||
| .key("BEHAVIOR_CONFIG_WITH_LEGACY_KEY") | ||
| .legacyKey("OLD_BEHAVIOR_CONFIG_WITH_LEGACY_KEY") | ||
| .description("Test behavior configuration with deprecated key") | ||
| .defaultValue("test-value") | ||
| .buildBehaviorChangeConfiguration(); | ||
|
|
||
| assertThat(config.hasLegacyKeys()).isTrue(); | ||
| assertThat(config.legacyKeys()).isEqualTo("OLD_BEHAVIOR_CONFIG_WITH_LEGACY_KEY"); | ||
| assertThat(config.key).isEqualTo("BEHAVIOR_CONFIG_WITH_LEGACY_KEY"); | ||
| assertThat(config.defaultValue).isEqualTo("test-value"); | ||
| } | ||
|
|
||
| @Test | ||
| void testDuplicateKeyValidation() { | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("DUPLICATE_KEY_TEST_1") | ||
| .description("First configuration") | ||
| .defaultValue(true) | ||
| .buildFeatureConfiguration(); | ||
|
|
||
| assertThatThrownBy( | ||
| () -> | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("DUPLICATE_KEY_TEST_1") | ||
| .description("Second configuration") | ||
| .defaultValue(false) | ||
| .buildFeatureConfiguration()) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("Config 'DUPLICATE_KEY_TEST_1' is already in use"); | ||
| } | ||
|
|
||
| @Test | ||
| void testDuplicateLegacyKeysValidation() { | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("NEW_KEY_DEPRECATED_TEST_2") | ||
| .legacyKey("LEGACY_KEY_TEST_2_1") | ||
| .legacyKey("LEGACY_KEY_TEST_2_2") | ||
| .legacyKey("LEGACY_KEY_TEST_2_3") | ||
| .description("First configuration with deprecated key") | ||
| .defaultValue(true) | ||
| .buildFeatureConfiguration(); | ||
|
|
||
| assertThatThrownBy( | ||
| () -> | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("ANOTHER_NEW_KEY_2") | ||
| .legacyKey("LEGACY_KEY_TEST_2_1") | ||
| .legacyKey("LEGACY_KEY_TEST_2_3") | ||
| .description("Second configuration with same deprecated key") | ||
| .defaultValue(false) | ||
| .buildFeatureConfiguration()) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("Config 'LEGACY_KEY_TEST_2_3,LEGACY_KEY_TEST_2_1' is already in use"); | ||
| } | ||
|
|
||
| @Test | ||
| void testNewKeyMatchingExistingLegacyKeyValidation() { | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("ORIGINAL_KEY_3") | ||
| .legacyKey("OLD_LEGACY_KEY_3") | ||
| .description("First configuration with deprecated key") | ||
| .defaultValue(true) | ||
| .buildFeatureConfiguration(); | ||
|
|
||
| assertThatThrownBy( | ||
| () -> | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("OLD_LEGACY_KEY_3") | ||
| .description("Configuration with key matching existing deprecated key") | ||
| .defaultValue(false) | ||
| .buildFeatureConfiguration()) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("Config 'OLD_LEGACY_KEY_3' is already in use"); | ||
| } | ||
|
|
||
| @Test | ||
| void testLegacyKeyMatchingExistingKeyValidation() { | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("EXISTING_KEY_4") | ||
| .description("First configuration") | ||
| .defaultValue(true) | ||
| .buildFeatureConfiguration(); | ||
|
|
||
| assertThatThrownBy( | ||
| () -> | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("NEW_KEY_FOR_VALIDATION_4") | ||
| .legacyKey("EXISTING_KEY_4") | ||
| .legacyKey("EXISTING_KEY_4_1") | ||
| .description("Configuration with deprecated key matching existing key") | ||
| .defaultValue(false) | ||
| .buildFeatureConfiguration()) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("Config 'EXISTING_KEY_4' is already in use"); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.