Dynamic Config Resolver #459
Conversation
andrew4699
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike,
ebyhr and
adutra
as code owners
|
|
||
| @ParameterizedTest | ||
| @MethodSource("getTestConfigs") | ||
| public void testPrecedenceIsDynamicThenStaticPerRealmThenStaticGlobal(TestConfig testConfig) { |
There was a problem hiding this comment.
What about catalog-level? I'd assume the catalog-level properties take precedence over everything, right?
There was a problem hiding this comment.
Based on this it already should. I assumed there were tests already for that.
| * 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 { |
There was a problem hiding this 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.
There was a problem hiding this 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.
|
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. |
Description
Introduces the notion of a
DynamicFeatureConfigResolver, which can be used to dynamically resolvefeatureConfigurationvalues 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
How Has This Been Tested?
Checklist: