refactor: store cleanup

[andreyturkov]

This PR

IStore interface usage instead of Store implementation
Separate locks for flag data and metadata in store - no longer valid point

[netlify]

✅ Deploy Preview for polite-licorice-3db33c canceled.

Name	Link
🔨 Latest commit	3e8aea6
🔍 Latest deploy log	https://app.netlify.com/projects/polite-licorice-3db33c/deploys/68a5a522ba6ab4000864d17f

[gemini-code-assist]

Summary of Changes

Hello @andreyturkov, I'm Gemini Code Assist¹! 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!

I've refactored the core store package to enhance its design and concurrency. The primary goal was to introduce and enforce the use of an IStore interface, abstracting the underlying implementation. This involved renaming the concrete Store struct to an unexported store and updating its constructor to return the interface. A significant improvement in concurrency was achieved by separating the single read-write mutex into two distinct mutexes, one for flag data and another for metadata, allowing for more efficient concurrent access. Additionally, I've centralized the logic for synchronizing source configurations by moving it into a new method on the IStore interface. These changes promote a more robust, testable, and maintainable store component by adhering to interface-driven design principles and improving internal concurrency.

Highlights

• Store Implementation Encapsulation: The Store struct has been renamed to store (lowercase) and is now unexported, ensuring that external packages interact with it only through the IStore interface.
• IStore Interface Expansion: The IStore interface has been extended with new methods, including GetMetadataForSource, Update, String, and SyncConfig, providing a more comprehensive contract for store operations.
• Finer-Grained Concurrency Control: The single sync.RWMutex (mx) has been replaced by two distinct sync.RWMutex instances: flagMx for flag data and metaMx for metadata. This allows for more granular locking, potentially reducing contention and improving performance for concurrent read/write operations on different parts of the store.
• Factory Function Returns Interface: The NewStore function now returns the IStore interface, reinforcing the pattern of programming against interfaces rather than concrete implementations.
• Centralized Source Configuration: The logic for configuring flag sources, previously handled directly in flagd/pkg/runtime/from_config.go, has been moved into a new SyncConfig method on the IStore interface and implemented in the store struct. This centralizes source configuration management within the store itself.
• Consumer Migration to IStore: All internal components that previously directly depended on *store.Store, such as the JSON evaluator and various sync services, have been updated to use the store.IStore interface, aligning with the new architectural design.

Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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 or fill out our survey to provide feedback.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request refactors the store to use an IStore interface instead of the concrete Store implementation, improving decoupling. It also introduces separate read-write locks for flag data and metadata. The changes are mostly clean and follow the stated goals. However, I've identified a race condition and some potentially unnecessary locking which I've detailed in my comments.

[beeme1mr]

Hey @andreyturkov, thanks for the PR. There's a fairly large refactor open right now that we're hoping to get in soon. I'd like to hold off on making this change until that one has been merged. FYI @toddbaert

[andreyturkov]

Hey @andreyturkov, thanks for the PR. There's a fairly large refactor open right now that we're hoping to get in soon. I'd like to hold off on making this change until that one has been merged. FYI @toddbaert

I'have rebased my branch with the latest main. Please take a look.

[beeme1mr]

The licensing issue was a false positive and I've added an exception in FOSSA.

[tangenti]

Could you also check the usages of lock in Store?

Now the lock should only be used for metadata, as flag CRUD is done with transaction. In String() for example, the lock is still used which is not needed I think

[andreyturkov]

Could you also check the usages of lock in Store?

Now the lock should only be used for metadata, as flag CRUD is done with transaction. In String() for example, the lock is still used which is not needed I think

You are absolutely right. The lock in the String() method is unnecessary as the underlying GetAll() call is transactional and doesn't rely on the mutex.
The same logic applies to the Update method.
I'll remove the mutex usage from both methods, and since it's no longer needed, I'll also remove the mutex field from the store struct itself.

[toddbaert]

Great work @andreyturkov , thanks! merging once the CI completes...

Do you have any interest in implementing this? It's something we believe is important; there's also room for an optional refactor if you're keen to that.