refactor: Decouple event publishing from Flagd Resolver when state changes #531
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* This will decouple the resolver and provider and ensure it happens asynchronously Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Kyle Julian <[email protected]>
kylejuliandev
changed the title
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the communication between FlagdProvider and the Resolver implementations to use a standard event-based pattern instead of passing callbacks in constructors. This effectively decouples the components, improving maintainability and adhering to good design principles. The changes are well-executed:
- The
Resolverinterface and its implementations (InProcessResolver,RpcResolver) now expose aProviderEvent. FlagdProvidersubscribes to this event upon initialization and correctly unsubscribes during shutdown to prevent potential memory leaks.- Compatibility with .NET Framework is preserved by conditionally having
FlagdProviderEventinherit fromEventArgs. - All associated tests have been updated to reflect these changes.
The refactoring is clean and improves the overall code structure. I have not found any issues of medium or higher severity.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR
Shortly after #515 was merged I realised that the approach I had taken had more explicitly coupled the Provider and Resolver to one another. By using an event we can subscribe and unsubscribe when necessary in the Provider. The state transitions are still being executed sychonrously on the same thread, so the behaviour should be unchanged, but hopefully this clean up the code and keeps a separation of concerns.
Related Issues
Notes
Follow-up Tasks
How to test