feat: move multi-provider into SDK and deprecate contrib implementation (open-feature#1486) #1765
Conversation
Summary of ChangesHello @Vishalup29, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the multi-provider functionality within the OpenFeature Java SDK. It transitions the multi-provider from a contrib module to a core SDK component, making it more accessible and integrated. The new implementation introduces flexible evaluation strategies and is clearly marked as experimental to manage expectations regarding API stability. This change streamlines multi-provider usage, improves test coverage, and updates documentation, while deprecating the older contrib version. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully moves the multi-provider implementation into the core SDK and marks it as experimental. The changes are well-structured, including new strategies and tests. My review focuses on a few critical resource management issues in the MultiProvider class, specifically related to ExecutorService lifecycle management. I've also provided suggestions to improve maintainability by reducing dependencies, clarifying code, and enhancing Javadoc documentation. Addressing these points will make the new experimental feature more robust and easier to maintain.
src/main/java/dev/openfeature/sdk/providers/multiprovider/MultiProvider.java
Outdated
Show resolved
Hide resolved
| public void shutdown() { | ||
| log.debug("shutdown begin"); | ||
| for (FeatureProvider provider : providers.values()) { | ||
| try { | ||
| provider.shutdown(); | ||
| } catch (Exception e) { | ||
| log.error("error shutdown provider {}", provider.getMetadata().getName(), e); | ||
| } | ||
| } | ||
| log.debug("shutdown end"); | ||
| } |
There was a problem hiding this comment.
The shutdown method overrides the one from EventProvider but doesn't call super.shutdown(). This means the emitterExecutor in the parent class will not be shut down, leading to a resource leak. You should call super.shutdown() to ensure all resources are properly cleaned up.
@Override
public void shutdown() {
log.debug("shutdown begin");
for (FeatureProvider provider : providers.values()) {
try {
provider.shutdown();
} catch (Exception e) {
log.error("error shutdown provider {}", provider.getMetadata().getName(), e);
}
}
log.debug("shutdown end");
super.shutdown();
}
src/main/java/dev/openfeature/sdk/providers/multiprovider/MultiProvider.java
Outdated
Show resolved
Hide resolved
| public Metadata getMetadata() { | ||
| return () -> metadataName; | ||
| } |
There was a problem hiding this comment.
Using a JSON string as the provider name is a creative workaround for the Metadata interface limitation. However, it might lead to unexpected behavior or difficult debugging if other parts of the system expect a simple name (e.g., in logs, metrics). For an experimental feature this might be acceptable, but it would be good to consider if the Metadata interface could be evolved in the future to support structured data.
src/main/java/dev/openfeature/sdk/providers/multiprovider/Strategy.java
Outdated
Show resolved
Hide resolved
Vishalup29
changed the title
Vishalup29
force-pushed
the
issue_1486
branch
from
0f1e6d2 to
b5f371d
Compare
|
@chrfwow @toddbaert Can you pls review when you get a chance? |
|
Hi, thanks for your contribution and interest in the SDK! We genuinely appreciate your work on multi-provider support. Since there's already a well-developed PR (#1500 ) addressing the same feature, having multiple PRs for the same goal creates extra effort for reviewers and can slow things down. Next time, we suggest building on the existing work - especially when significant development and review have already occurred. A few differences to keep in mind:
Your contributions are always welcome, but i think basing the work on the previous PR would be a good approach here. Especially to also credit the previous contributor of his base work. Thank you for your effort, can you maybe incorporate the already existing feedback from the other pull request, or re-base your efforts based on the other one. This would be highly appreciated. |
Vishalup29
force-pushed
the
issue_1486
branch
from
b5f371d to
ce61f2f
Compare
…ental, and deprecate contrib implementation. Signed-off-by: vishalup29 <[email protected]>
Vishalup29
force-pushed
the
issue_1486
branch
from
ce61f2f to
3cd33f4
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1765 +/- ##
============================================
+ Coverage 92.53% 94.08% +1.55%
- Complexity 599 634 +35
============================================
Files 55 58 +3
Lines 1406 1504 +98
Branches 154 165 +11
============================================
+ Hits 1301 1415 +114
+ Misses 64 50 -14
+ Partials 41 39 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully moves the multi-provider implementation from the contrib repository into the core SDK and marks it as experimental. The changes are well-implemented, including the MultiProvider class, different evaluation strategies (FirstMatchStrategy, FirstSuccessfulStrategy), and parallel initialization of providers for better performance. The error handling is robust, and the new feature is properly documented in the README.md. The test coverage for the new functionality is also comprehensive. I have one minor suggestion regarding Java naming conventions in the test packages.
| @@ -0,0 +1,214 @@ | |||
| package dev.openfeature.sdk.multiProvider; | |||
There was a problem hiding this comment.
The package name dev.openfeature.sdk.multiProvider does not follow Java's package naming conventions, which recommend all-lowercase names. The corresponding production code in src/main/java/dev/openfeature/sdk/multiprovider uses the correct lowercase naming. For consistency and to adhere to standard Java practices, this package should be renamed to dev.openfeature.sdk.multiprovider.
This change should be applied to all new test files in this pull request that share this package declaration (FirstMatchStrategyTest.java, FirstSuccessfulStrategyTest.java, and MultiProviderTest.java).
| package dev.openfeature.sdk.multiProvider; | |
| package dev.openfeature.sdk.multiprovider; |
2 participants
Description
Move the existing multi-provider implementation from the contrib module into the core Java SDK, mark it as experimental, and deprecate the contrib implementation.
Changes
MultiProvider(and related strategy/utility classes) fromjava-sdk-contrib/providers/multiproviderinto the core SDK underdev.openfeature.sdk.providers.multiprovider.README.mdto document the multi-provider feature and show a basic usage example.new FlagNotFoundError()rather than a message constructor).Related Issues
Fixes #1486
Notes
FlagNotFoundErroronly exposes a no-arg constructor, so the ported multi-provider now throwsnew FlagNotFoundError()where the contrib version used a message.