-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: multi-provider implementation #1028
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: Liran M <[email protected]>
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.
Thank you, this looks really cool. But especially the Strategy initialization and the handling of 'exception-less Errors' might be problematic
Function<FeatureProvider, ProviderEvaluation<T>> providerFunction) { | ||
for (FeatureProvider provider: getProviders().values()) { | ||
try { | ||
return providerFunction.apply(provider); |
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.
Not all providers throw exceptions sometimes, they return the information in the provider evaluation instead of throwing as exceptions for flows is expensive see open-feature/java-sdk#1095 - applies to all strategies
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.
updated accordingly.
@Slf4j | ||
public class FirstMatchStrategy extends BaseStrategy { | ||
|
||
public FirstMatchStrategy(Map<String, FeatureProvider> providers) { |
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.
This might be a problem, as the map is generated in the multi-provider; we need these values to be generated before we pass the strategy to the provider. Maybe we should also put those as parameters to the evaluate
method else it will be hard to use any other strategy than the default
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 updated it a bit. The providers can be reached via BaseStrategy, as done at MultiProviderTest. What do you think ?
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 am not sure, now we do have to separate maps where the state needs to be in sync. this might cause problems. we should centralize this on the provider, what if we change the signature of the evaluate
method like
public interface Strategy {
<T> ProviderEvaluation<T> evaluate(Map<...> providers, String key, T defaultValue, EvaluationContext ctx,
Function<FeatureProvider, ProviderEvaluation<T>> providerFunction);
}
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.
it is commonly the same providers instance at the multi-provider and the strategy, and it is built as unmodifiable when initialized. Why can this be a problem ? I am fine with your approach above too, just want to understand better first.
another thing, reg. ProviderEvaluation> providerFunction as parameter:
it is pretty straight-forward to implement with it, but I have some concerns that if a custom strategy implementation want to do change the function call and/or arguments, it will limit it. Other approach can be to pass the type. e.g. Boolean.class instead of the function, and convert it to the specific evaluation method. What do you think ?
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.
if i eg. switch to the FirstSuccessful implementation my code would look like
var providers = List.of(...);
var strategy = new FirstSuccessful(providers);
var provider = new MultiProvider(providers, strategy);
but that means that theoretically the providers list could be a different one, or am i missing here something?
Regarding your providerFunction, i do think that this is the clearest way also maintainence wise - trying to hassle with type information adds unnecessary overhead in my opinion
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.
ok, updated accordingly.
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
providers/multiprovider/CHANGELOG.md
Outdated
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.
Please remove this changelog. A new one should be created by Release Please.
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.
done
release-please-config.json
Outdated
@@ -100,6 +100,17 @@ | |||
"README.md" | |||
] | |||
}, | |||
"providers/multi-provider": { |
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.
The file path is providers/multiprovider
. Please either update the path name or this configuration.
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.
done
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.
BTW, originally I had "multi-provider" like other providers, but it failed validation on Invalid Automatic-Module-Name:
❌ no - allowed
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.
Why would Maven Central allow invalid names to be uploaded?
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.
Maybe it's because they aren't invalid in other JVM langauges, like Scala or Kotlin, and maven supports all of these.
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
|
||
@SneakyThrows | ||
@Test | ||
public void testInit() { |
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 we need 2 more tests for init:
- one where both providers implement a successful init() function
- one where one provider's init functions throws (and init should throw).
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.
added
*/ | ||
@Override | ||
public void initialize(EvaluationContext evaluationContext) throws Exception { | ||
JSONObject json = new JSONObject(); |
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.
Instead of JSON object, could we use a HashMap
, or our SDK's own build-in object representation (Structure and it's implementations)?
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.
According to Multi-provider metadata spec, it is expected to be printed as JSON, which makes sense.
Both HashMap and Structure not printed as Json.
JSONObject providersMetadata = new JSONObject(); | ||
json.put("originalMetadata", providersMetadata); | ||
for (FeatureProvider provider: providers.values()) { | ||
provider.initialize(evaluationContext); |
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.
We could probably initialize in parallel with some threads; not a blocker for me but if you initialize 3 providers that needed to setup remote connections, you could be waiting a while before you get successful failure.
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.
done
@@ -0,0 +1,97 @@ | |||
# OpenFeature Multi-Provider for Java |
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.
Nice work on the README.
Small nit: 1 sentence per line makes reviewing and change-deltas much nicer.
Markdown will add it's own line-breaks when it renders.
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.
Thanks, updated.
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 left a few comments but overall looks very close.
Signed-off-by: liran2000 <[email protected]>
Readme describes the provider.
References