Add RealmConfig #2015
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| /* | ||
| * 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 jakarta.annotation.Nullable; | ||
| import org.apache.polaris.core.entity.CatalogEntity; | ||
|
|
||
| /** Realm-specific configuration used to retrieve runtime parameters. */ | ||
| public interface RealmConfig { | ||
|
|
||
| /** | ||
| * Retrieve the current value for a configuration key. May be null if not set. | ||
| * | ||
| * @param <T> the type of the configuration value | ||
| * @param configName the name of the configuration key to check | ||
| * @return the current value set for the configuration key, or null if not set | ||
| */ | ||
| <T> @Nullable T getConfig(String configName); | ||
|
|
||
| /** | ||
| * Retrieve the current value for a configuration key. If not set, return the non-null default | ||
| * value. | ||
| * | ||
| * @param <T> the type of the configuration value | ||
| * @param configName the name of the configuration key to check | ||
| * @param defaultValue the default value if the configuration key has no value | ||
| * @return the current value or the supplied default value | ||
| */ | ||
| <T> T getConfig(String configName, T defaultValue); | ||
|
|
||
| /** | ||
| * Retrieve the current value for a configuration. | ||
| * | ||
| * @param <T> the type of the configuration value | ||
| * @param config the configuration to load | ||
| * @return the current value set for the configuration key or null if not set | ||
| */ | ||
| <T> T getConfig(PolarisConfiguration<T> config); | ||
|
|
||
| /** | ||
| * Retrieve the current value for a configuration, overriding with a catalog config if it is | ||
| * present. | ||
| * | ||
| * @param <T> the type of the configuration value | ||
| * @param config the configuration to load | ||
| * @param catalogEntity the catalog to check for an override | ||
| * @return the current value set for the configuration key or null if not set | ||
| */ | ||
| <T> T getConfig(PolarisConfiguration<T> config, CatalogEntity catalogEntity); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| /* | ||
| * 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 jakarta.annotation.Nullable; | ||
| import org.apache.polaris.core.context.RealmContext; | ||
| import org.apache.polaris.core.entity.CatalogEntity; | ||
|
|
||
| public class RealmConfigImpl implements RealmConfig { | ||
|
|
||
| private final PolarisConfigurationStore configurationStore; | ||
|
There was a problem hiding this comment. If a new There was a problem hiding this comment. the idea of having the interface is to make the life of the callers easier as they most often (/always?) operate in the context of a single realm / whether configuration values are stored by a single store for all/multiple realms or not is an implementation detail that callers should not care about. There was a problem hiding this comment.
Isn't that a reason that you'd want to just inject a There was a problem hiding this comment.
There was a problem hiding this comment. I'm all for refactoring There was a problem hiding this comment. The new layer helps to properly delineate application-scoped data ( There was a problem hiding this comment. Not having to deal with realms is definitely a great improvement, agreed. But my concern remains that this new class becomes the de facto “Polaris Configuration Store” and this redundancy does not improve clarity. Furthermore, the proposed structure could be interpreted as implying that these configs are set per-realm (while PolarisConfigurationStore confits are not) when in fact this is not the case. |
||
| private final RealmContext realmContext; | ||
|
|
||
| public RealmConfigImpl(PolarisConfigurationStore configurationStore, RealmContext realmContext) { | ||
| this.configurationStore = configurationStore; | ||
| this.realmContext = realmContext; | ||
| } | ||
|
|
||
| @Override | ||
| public <T> @Nullable T getConfig(String configName) { | ||
| return configurationStore.getConfiguration(realmContext, configName); | ||
| } | ||
|
|
||
| @Override | ||
| public <T> T getConfig(String configName, T defaultValue) { | ||
| return configurationStore.getConfiguration(realmContext, configName, defaultValue); | ||
| } | ||
|
|
||
| @Override | ||
| public <T> T getConfig(PolarisConfiguration<T> config) { | ||
| return configurationStore.getConfiguration(realmContext, config); | ||
| } | ||
|
|
||
| @Override | ||
| public <T> T getConfig(PolarisConfiguration<T> config, CatalogEntity catalogEntity) { | ||
| return configurationStore.getConfiguration(realmContext, catalogEntity, config); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really a
Realm-specific configuration? This looks more like a configuration store, likePolarisConfigurationStore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depends on one's definition i guess, currently the interface allows retrieval of configuration values. in my book that makes it a "configuration".
do you want me to add "store" to the javadoc or to also rename the class? if the latter, any concrete name suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think calling it a RealmConfigurationStore would be a better name, but that also only makes me think more that this needs to be reconciled with PolarisConfigurationStore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not mind the proposed renaming, but current name LGTM too.