feat: Make the Evaluator customizable#1733
Conversation
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
Summary of ChangesHello, 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 introduces the capability to customize the Highlights
Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable enhancement by making the Evaluator customizable in FlagdOptions. This allows users to provide their own flag evaluation logic for the in-process resolver. The implementation is clean, and the new functionality is well-covered by unit tests.
My main feedback, which is included as a specific comment, is to improve the documentation for the new evaluator option to include thread-safety requirements, as custom implementations will be called from multiple threads concurrently. This will help prevent potential race conditions for users of this new feature.
| /** | ||
| * The evaluator to use for flag evaluations. Defaults to {@code new FlagdCore()}. Only applicable in the in-process | ||
| * mode | ||
| */ |
There was a problem hiding this comment.
It's important to document the thread-safety requirements for custom Evaluator implementations. The setFlags method is called by an internal streamer thread, while the resolve*Value methods are called by application threads. Without this information in the Javadoc, users might provide a non-thread-safe implementation, leading to potential race conditions.
| /** | |
| * The evaluator to use for flag evaluations. Defaults to {@code new FlagdCore()}. Only applicable in the in-process | |
| * mode | |
| */ | |
| /** | |
| * The evaluator to use for flag evaluations. Defaults to {@code new FlagdCore()}. | |
| * Only applicable in the in-process mode. | |
| * <p> | |
| * <b>Note:</b> Custom implementations of {@link Evaluator} must be thread-safe, as | |
| * methods can be called concurrently from multiple threads. | |
| */ |
There was a problem hiding this comment.
This is not a concern of a builder method
|
i think we should also add the SPI option, if a a evaluator is available, we use it via ServiceLoader, but the configuration via flagdOptions takes precedence over SPI. |
|
@aepfli is SPI already setup for this project? I found no module.info file in the current state |
This PR
Makes the evaluator configurable via the FlagdOptions
Related Issues
Fixes #1732