You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "andrew4699 (via GitHub)" <gi...@apache.org> on 2024/11/20 00:53:48 UTC
[PR] Dynamic Config Resolver [polaris]
andrew4699 opened a new pull request, #459:
URL: https://github.com/apache/polaris/pull/459
# Description
Introduces the notion of a `DynamicFeatureConfigResolver`, which can be used to dynamically resolve `featureConfiguration` values at runtime. This is intended for integration with feature flag systems (Statsig, LaunchDarkly, etc) but this PR does not provide the implementations, rather just a hook to make it possible.
## Type of change
- [X] New feature (non-breaking change which adds functionality)
# How Has This Been Tested?
- [X] Unit tests
- [ ] TODO: Will test end-to-end with a sample implementation
# Checklist:
- [X] I have performed a self-review of my code
- [X] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [X] My changes generate no new warnings
- [ ] If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2498711198
The example is pretty easy:
```
@Inject MyConfig myConfig;
```
Where `MyConfig` would be sth like
```
public interface MyConfig {
public int someIntValue();
public OtherThing other();
public Duration someDuration();
}
```
The rest is just smallrye-config setup and usage stuff - some starter information is [here](https://quarkus.io/guides/config-reference#default-values). Config sources do not have to be static - those can provide dynamic values - but that's up to the config source really.
I'd really prefer an approach that leverages functionality that is proven to work in the wild for quite some time now, and with Quarkus there are already a lot of extensions that do provide "dynamic configuration" - for example [this one](https://docs.quarkiverse.io/quarkus-unleash/dev/index.html) or [this one](https://docs.quarkiverse.io/quarkus-vault/dev/index.html). Using already existing functionality is IMHO more efficient, think of #469, than writing some custom configuration framework.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on code in PR #459:
URL: https://github.com/apache/polaris/pull/459#discussion_r1854326839
##########
polaris-service/src/main/java/org/apache/polaris/service/config/DynamicFeatureConfigResolver.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.service.config;
+
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import io.dropwizard.jackson.Discoverable;
+import java.util.Optional;
+
+/**
+ * DynamicFeatureConfigResolvers dynamically resolve featureConfigurations. This is useful for
+ * integration with feature flag systems which are intended for fetching configs at runtime.
+ */
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type")
+public interface DynamicFeatureConfigResolver extends Discoverable {
Review Comment:
Another option would be to make the whole implementation swappable as you mentioned, which could be implemented in a backward compatible way: if a custom type is specified, use it, otherwise use the default. I don't think this change makes that impossible in the future. In fact, you can simply make DynamicFeatureConfigResolver implement the entire configuration store.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 closed pull request #459: Dynamic Config Resolver
URL: https://github.com/apache/polaris/pull/459
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2498644065
@snazy Snowflake is interested in the capability. This is pretty much the simplest and most flexible implementation possible. It can exist regardless of the framework as the implementation can fetch config from anywhere. Can you share an example of how you could inject dynamic, account-level config from a 3rd party in the Quarkus PR?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2500016857
> Another option would be to make the whole implementation swappable as you mentioned, which could be implemented in a backward compatible way: if a custom type is specified, use it, otherwise use the default.
I also would prefer this approach, as it is CDI-friendly.
But I also think that, before diving head first on making things "dynamic", we should take some time to design a revamped `PolarisConfigurationStore` SPI that would address all the requirements. I've seen the below requirements already voiced (there may be others):
* Ability to (optionally?) hot-reload.
* Ability to merge layers of configuration: application > realm > namespace > table.
* Ability to swap implementations (mostly to fetch configuration from different sources).
But the current `PolarisConfigurationStore` API does not surface any of that. E.g. there is no `hotReload()` method; there is no `merge()` method. Moreover, none of the methods take a `realm`, or a `table` as argument – instead, the realm is "implicitly passed" through a `ThreadLocal` in `CallContext`, and becomes an implementation detail. This makes me think that `PolarisConfigurationStore` needs a complete redesign.
On top of that, I always wondered why `PolarisConfiguration` has such a strange shape. Instead of a classic config bean:
```java
interface PolarisConfiguration {
boolean configX();
Optional<String> configY();
List<String> configZ();
}
```
... that each implementor would be free to implement their own way, we have:
```java
public class PolarisConfiguration<T> {
public static final PolarisConfiguration<Boolean> CONFIG_X = ...
public static final PolarisConfiguration<String> CONFIG_Y = ...
public static final PolarisConfiguration<List<String> CONFIG_Z = ...
}
```
... which is a lot harder to extend, or to make "dynamic" or "layered". And involves many runtime casts that are error-prone, cf `PolarisConfigurationStore#tryCast()`. So, again, I think `PolarisConfiguration` also needs a complete redesign.
I would therefore suggest that we embrace the big picture and redesign `PolarisConfigurationStore` and `PolarisConfiguration` with all the above considerations and requirements in mind, before adding more features such as "dynamic resolution".
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2499158130
> Another option would be to make the whole implementation swappable as you mentioned, which could be implemented in a backward compatible way: if a custom type is specified, use it, otherwise use the default.
I think this is a reasonable approach and I think this should address the concern of _where_ we get the dynamic config. A Quarkus impl can use whatever the smallrye config structure internally, but we can't tie it to that. The `PolarisConfigurationStore` is intended to support feature rollouts and feature configuraiton globally, but also to be fine-tuned at the realm-level or catalog level. Ultimately, I think it's going to need to allow for namespace and table-level configuration as well. Configuration should be inheritable, so it'll have to have the concept of table-lineage (i.e., table inherits from namspace inherits from catalog inherits from service) and we're just not going to see that in a generalized configuration framework. I'm all for not reinventing the wheel, but we shouldn't hand-wave over the details and pretend it all looks like a configuration file parser.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on code in PR #459:
URL: https://github.com/apache/polaris/pull/459#discussion_r1854305841
##########
polaris-service/src/test/java/org/apache/polaris/service/config/DefaultConfigurationStoreTest.java:
##########
@@ -106,4 +111,77 @@ public void testGetRealmConfiguration() {
String keyTwoRealm3 = defaultConfigurationStore.getConfiguration(realm3Ctx, "key2");
assertThat(keyTwoRealm3).isEqualTo(defaultKeyTwoValue);
}
+
+ @Test
+ public void testDynamicConfig() {
+ InMemoryPolarisMetaStoreManagerFactory metastoreFactory =
+ new InMemoryPolarisMetaStoreManagerFactory();
+ PolarisCallContext polarisCtx =
+ new PolarisCallContext(
+ metastoreFactory.getOrCreateSessionSupplier(() -> "realm1").get(),
+ new PolarisDefaultDiagServiceImpl());
+
+ String key = "k1";
+ Map<String, Object> staticConfig = Map.of(key, 10);
+
+ assertThat(
+ new DefaultConfigurationStore(staticConfig, Map.of(), k -> Optional.empty())
+ .<Integer>getConfiguration(polarisCtx, key))
+ .as("The DynamicFeatureConfigResolver always returns Optional.empty()")
+ .isEqualTo(10);
+
+ assertThat(
+ new DefaultConfigurationStore(staticConfig, Map.of(), k -> Optional.of(5))
+ .<Integer>getConfiguration(polarisCtx, key))
+ .as("The DynamicFeatureConfigResolver always returns 5")
+ .isEqualTo(5);
+ }
+
+ @ParameterizedTest
+ @MethodSource("getTestConfigs")
+ public void testPrecedenceIsDynamicThenStaticPerRealmThenStaticGlobal(TestConfig testConfig) {
Review Comment:
Based on [this](https://github.com/apache/polaris/blob/d20cccd41da1c6ea554b4958619ed9863eeed3b6/polaris-core/src/main/java/org/apache/polaris/core/PolarisConfigurationStore.java#L112-L116) it already should. I assumed there were tests already for that.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #459:
URL: https://github.com/apache/polaris/pull/459#discussion_r1852940725
##########
polaris-service/src/test/java/org/apache/polaris/service/config/DefaultConfigurationStoreTest.java:
##########
@@ -106,4 +111,77 @@ public void testGetRealmConfiguration() {
String keyTwoRealm3 = defaultConfigurationStore.getConfiguration(realm3Ctx, "key2");
assertThat(keyTwoRealm3).isEqualTo(defaultKeyTwoValue);
}
+
+ @Test
+ public void testDynamicConfig() {
+ InMemoryPolarisMetaStoreManagerFactory metastoreFactory =
+ new InMemoryPolarisMetaStoreManagerFactory();
+ PolarisCallContext polarisCtx =
+ new PolarisCallContext(
+ metastoreFactory.getOrCreateSessionSupplier(() -> "realm1").get(),
+ new PolarisDefaultDiagServiceImpl());
+
+ String key = "k1";
+ Map<String, Object> staticConfig = Map.of(key, 10);
+
+ assertThat(
+ new DefaultConfigurationStore(staticConfig, Map.of(), k -> Optional.empty())
+ .<Integer>getConfiguration(polarisCtx, key))
+ .as("The DynamicFeatureConfigResolver always returns Optional.empty()")
+ .isEqualTo(10);
+
+ assertThat(
+ new DefaultConfigurationStore(staticConfig, Map.of(), k -> Optional.of(5))
+ .<Integer>getConfiguration(polarisCtx, key))
+ .as("The DynamicFeatureConfigResolver always returns 5")
+ .isEqualTo(5);
+ }
+
+ @ParameterizedTest
+ @MethodSource("getTestConfigs")
+ public void testPrecedenceIsDynamicThenStaticPerRealmThenStaticGlobal(TestConfig testConfig) {
Review Comment:
What about catalog-level? I'd assume the catalog-level properties take precedence over everything, right?
##########
polaris-service/src/main/java/org/apache/polaris/service/config/DynamicFeatureConfigResolver.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.service.config;
+
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import io.dropwizard.jackson.Discoverable;
+import java.util.Optional;
+
+/**
+ * DynamicFeatureConfigResolvers dynamically resolve featureConfigurations. This is useful for
+ * integration with feature flag systems which are intended for fetching configs at runtime.
+ */
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type")
+public interface DynamicFeatureConfigResolver extends Discoverable {
Review Comment:
TBH, I feel that this type _should_ be unnecessary. Unfortunately, we just added the `featureConfiguration` field as a map, rather than having the `DefaultConfigurationStore` instantiated and populated by Jackson during application startup. Now the configuration can't really change to add, e.g., a `type` configuration key so that we could use different `PolarisConfigurationStore` implementations. The `PolarisConfigurationStore` interface was always intended to support dynamic implementations - which is why the `PolarisCallContext` is part of the signature.
I'm just trying to figure out if there isn't a way to make the implementation swappable, while still supporting the backward-compatible configuration. I mean, there's no real harm in adding another interface, but it just feels unnecessary and the consequence of poor decision making earlier on. I hate the idea of that poor decision making having to follow us around forever.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2497388047
> Hey @snazy I'm not sure I understand the intent of the question. This is the only change I have planned regarding dynamic configs, as this provides the hook necessary for implementors.
My question is: what/who needs this change?
My overall concern is it's going beyond a "simple" implementation. Maybe I'm a bit blessed as I'm used to what Quarkus and all its extensions offer for configuration via Smallrye-Config, where such things (various config sources, dynamic configurations, encryption, etc etc) are rather just available.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2498717651
NB: you can take a look on https://github.com/apache/polaris/pull/469 where we are using injected confict.
Especially: https://github.com/jbonofre/polaris/blob/QUARKUS/polaris-service-quarkus/README.md#configuration
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2689137433
Closing this for now as the branch is out of date and the ConfigurationStore is swappable
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2494333670
> What are the things that need / depend on this change?
Hey @snazy I'm not sure I understand the intent of the question. This is the only change I have planned regarding dynamic configs, as this provides the hook necessary for implementors.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2493572261
What are the things that need / depend on this change?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2563231619
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Dynamic Config Resolver [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #459:
URL: https://github.com/apache/polaris/pull/459#issuecomment-2566323355
FYI, Quarkus has recently introduced a [new extension](https://github.com/quarkusio/quarkus/issues/44171) that integrates Quarkus with [Open Feature](https://openfeature.dev), a backend-agnostic feature flag library. Open Feature has support for Statsig, Unleash, flagd and a few others (but not LaunchDarkly apparently).
This is all bleeding edge for sure, but we may want to explore this approach as well, as opposed to implementing our own from scratch.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org