-
Notifications
You must be signed in to change notification settings - Fork 46
feat: Add multi-provider support #1500
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
base: main
Are you sure you want to change the base?
feat: Add multi-provider support #1500
Conversation
8710a52
to
f07dd25
Compare
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.
Looks good so far!
I'm not sure if we want to add a json dependecy (which might also produce the License Compliance error) "just" so that we can build up the metadata name string.
private String metadataName; | ||
|
||
/** | ||
* Constructs a MultiProvider with the given list of FeatureProviders, using a default strategy. |
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 the comment should say what the default strategy is
json.put("name", NAME); | ||
JSONObject providersMetadata = new JSONObject(); | ||
json.put("originalMetadata", providersMetadata); | ||
ExecutorService initPool = Executors.newFixedThreadPool(INIT_THREADS_COUNT); |
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.
ExecutorService initPool = Executors.newFixedThreadPool(INIT_THREADS_COUNT); | |
ExecutorService initPool = Executors.newFixedThreadPool(Math.min(INIT_THREADS_COUNT, providers.size())); |
throw new GeneralError("init failed"); | ||
} | ||
} | ||
metadataName = json.toString(); |
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 the executer service is unused from this point onwards, we should shut it down
assertThrows(FlagNotFoundError.class, () -> finalMultiProvider1.getStringEvaluation("non-existing", "", null)); | ||
|
||
multiProvider.shutdown(); | ||
multiProvider = new MultiProvider(providers, new FirstSuccessfulStrategy()); |
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.
There should probably be a test file with tests for each strategy. If you want to reduce code duplication by the flag setup, make those test classes extend some common test setup class
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.
have created a BaseStrategyTest
for the setup which is extended by the separate test classes for each strategy
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1500 +/- ##
============================================
+ Coverage 92.81% 93.41% +0.60%
- Complexity 486 518 +32
============================================
Files 46 50 +4
Lines 1169 1246 +77
Branches 103 112 +9
============================================
+ Hits 1085 1164 +79
+ Misses 54 51 -3
- Partials 30 31 +1
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:
|
Should I construct the json using Strings? |
Maybe it would be worth it. |
@chrfwow @toddbaert any updates on this? |
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, great effort. I am looking forward to having the multiprovider migrated. For me, the added dependency is currently a show stopper, because I think we can handle that with proper and simple data classes, even with better metadata support overall (not losing information from the sub providers). I added simple but untested code snippets to my review. Please let me know what you think about this approach.
public static final int INIT_THREADS_COUNT = 8; | ||
private final Map<String, FeatureProvider> providers; | ||
private final Strategy strategy; | ||
private String metadataName; |
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.
[suggestion] as this variable is more than just a name - it is a json object, we might want to rename it to something more representing like
private String metadataName; | |
private String metadata; |
*/ | ||
@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.
[nit] If this is really the only reason why we add a JSON dependency, can we maybe utilize our own object representation of metadata?
something like
@Override
public void initialize(EvaluationContext evaluationContext) throws Exception {
var metadataBuilder = MultiProviderMetadata.builder();
metadataBuilder.name(NAME);
Map<String, Metadata> providersMetadata = new HashMap();
ExecutorService initPool = Executors.newFixedThreadPool(INIT_THREADS_COUNT);
Collection<Callable<Boolean>> tasks = new ArrayList<>(providers.size());
for (FeatureProvider provider : providers.values()) {
tasks.add(() -> {
provider.initialize(evaluationContext);
return true;
});
Metadata providerMetadata = provider.getMetadata();
providersMetadata.put(providerMetadata.getName(), providerMetadata);
}
metadataBuilder.originalMetadata(providersMetadata);
List<Future<Boolean>> results = initPool.invokeAll(tasks);
for (Future<Boolean> result : results) {
if (!result.get()) {
throw new GeneralError("init failed");
}
}
metadata = metadataBuilder.build();
}
with an own implementation of the metadata interface called MultiProviderMetadata
@Data
@Builder
public class MultiProviderMetadata implements Metadata {
String name;
Map<String, Metadata> originalMetadata;
}
wdyt? This would eliminate the dependency and make editing the code easier - I did not test this, and I am not sure if we will miss information with JSON conversion, etc.
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 getMetaData
function is supposed to return a String, what should I be returning in this implementation?
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 you can just return the metadata object.
@Override
public Metadata getMetadata() {
return metadata;
}
There is no need for the lambda anymore :)
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 are we using this map - Map<String, Metadata> originalMetadata;
again?
MetaData will only contain one field that is the name right?
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.
For the Multiprovider it is different. Ideally the metadata contains all the metadata of each provider too. So this map will contain for each sub provider the metadata
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'm getting - may expose internal representation on Spotbugs for getMetaData.
Was thinking of creating a defensive copy in getMetaData, something like this -
public Metadata getMetadata() {
if (metadata == null) {
return null;
}
Map<String, Metadata> defensiveCopy = metadata.getOriginalMetadata() == null
? null
: Map.copyOf(metadata.getOriginalMetadata());
return MultiProviderMetadata.builder()
.name(metadata.getName())
.originalMetadata(defensiveCopy)
.build();
}
Is this acceptable ?
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.
to be honest, i think we should ignore this spotbugs warning
Looks good to me. |
Signed-off-by: suvaidkhan <[email protected]>
… removed json dep Signed-off-by: suvaidkhan <[email protected]>
97f6dc9
to
1103949
Compare
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.
Looks good
List<Future<Boolean>> results = initPool.invokeAll(tasks); | ||
for (Future<Boolean> result : results) { | ||
if (!result.get()) { | ||
throw new GeneralError("init failed"); |
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 also need to shut down the executer service in this case
Strategy mockStrategy = mock(Strategy.class); | ||
MultiProvider multiProvider = new MultiProvider(providers, mockStrategy); | ||
multiProvider.initialize(null); | ||
assertNotNull(multiProvider); |
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 assert is a NO-OP
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.
assertNotNull(multiProvider); |
Signed-off-by: suvaidkhan <[email protected]>
…port' into suvaidkhan/add-multiprovider-support
|
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, the basic implementation looks good to me. But i took a closer look at the two strategies. The strategies seem to be a little inconsistent in exception handling, logging and returning. I think we should normalize this a little for consistency (no need for more abstraction, but it should log the same way, it should handle all the possible exceptions the same way, etc).
Strategy mockStrategy = mock(Strategy.class); | ||
MultiProvider multiProvider = new MultiProvider(providers, mockStrategy); | ||
multiProvider.initialize(null); | ||
assertNotNull(multiProvider); |
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.
assertNotNull(multiProvider); |
var metadataBuilder = MultiProviderMetadata.builder(); | ||
metadataBuilder.name(NAME); | ||
HashMap<String, Metadata> providersMetadata = new HashMap<>(); | ||
ExecutorService initPool = Executors.newFixedThreadPool(Math.min(INIT_THREADS_COUNT, providers.size())); |
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.
ExecutorService initPool = Executors.newFixedThreadPool(Math.min(INIT_THREADS_COUNT, providers.size())); | |
ExecutorService executorService = Executors.newFixedThreadPool(Math.min(INIT_THREADS_COUNT, providers.size())); |
initPool
seems odd, we should properly name it, and i suggest to name it executorService
return res; | ||
} | ||
} catch (FlagNotFoundError e) { | ||
log.debug("flag not found {}", e.getMessage()); |
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.
log.debug("flag not found {}", e.getMessage()); | |
log.debug("flag not found {}", key, e); |
The exception might have a different message, hence i think adding the key is worth it, as a separate infomration. Additionally most logging frameworks allow you to add the exception as an additional parameter, which will print the stacktrace if available. It is just a good practice
log.debug("flag not found {}", e.getMessage()); | ||
} | ||
} | ||
throw new FlagNotFoundError("flag not found"); |
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 we throw an exception here i suggest to add the key
throw new FlagNotFoundError("flag not found"); | |
throw new FlagNotFoundError("flag '" + key + "' not found"); |
but it would be better to construct a ProviderEvaluation object with an ErrorCode. we do not use the exception for flow control, our SDK will also handle this properly.
} | ||
} | ||
|
||
throw new GeneralError("evaluation error"); |
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 should be consistent in our Exceptions/return-values - we should construct a ProviderEvaluation with an error code, and the error code should be consistent for all Strategies
This PR
Related Issues
Resolves #1486
Follow-up Tasks
Multiprovider should be removed from the contrib codebase