feat(flagd): add FILE resolver type for local flag definition files#647
feat(flagd): add FILE resolver type for local flag definition files#647aedeny wants to merge 4 commits into
Conversation
|
Related to #308 |
github-actions
Bot
requested review from
askpt,
bacherfl,
kylejuliandev and
toddbaert
![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 introduces a file-based resolver for the flagd provider, allowing flag definitions to be loaded from a local JSON file. Key additions include the FileBasedResolver, a custom FileSystemHashWatcher for reliable change detection in containerized environments, and corresponding configuration options via code and environment variables. Review feedback highlights several opportunities to improve memory efficiency and performance, such as reading and hashing file streams directly rather than using intermediate memory buffers, and optimizing directory searches during file system cache refreshes.
|
Hey @aedeny, thanks for contributing this change! I've noticed the DCO status is failing. Commits need to be signed off with To add your Signed-off-by line to every commit in this branch:
|
Add support for reading flagd feature flag definitions directly from a local file, with automatic change detection and live reload. New resolver: - FileBasedResolver: reads a flagd flag definition JSON file from disk, validates it against the JSON schema, and evaluates flags locally via JsonEvaluator. Supports waiting for the file to appear (configurable timeout, default 5 min) and reloading on changes. - FileSystemHashWatcher: polls file content using MurmurHash-128 to detect changes. Designed for environments where filesystem events are unreliable (e.g., NFS, container-mounted volumes). Configurable polling interval (default 1 min). Baseline hash is computed synchronously on Start() to prevent race conditions. - When hash-based detection is not enabled, a standard FileSystemWatcher with 500ms debounce is used instead. Configuration via FLAGD_RESOLVER=file, FLAGD_SOURCE_FILE_PATH, and FLAGD_HASH_FILE_CHANGE env vars. Builder API: WithSourceFilePath(), WithUseHashFileChangeDetection(). DI options: FlagdProviderOptions.SourceFilePath, FlagdProviderOptions.UseHashFileChangeDetection. Error handling: FileShare.ReadWrite|Delete for safe concurrent access, schema validation on Init, ProviderError events on parse failures without crashing, ReaderWriterLockSlim protects evaluator during reloads. Tests: 31 new tests (201 total pass) covering FileBasedResolver lifecycle and resolution, FileSystemHashWatcher polling, FlagdConfig env var parsing, FlagdProvider instantiation, and DI option mapping. Documentation: Updated README with FILE resolver config table entries, usage examples, and hash-based detection docs. Signed-off-by: Eden Yefet <edenyefet@gmail.com>
When a targeting rule evaluates to null (no match), the resolver correctly falls back to the defaultVariant value but was returning variant: null in ResolutionDetails instead of the defaultVariant name. Assign variant = flagConfiguration.DefaultVariant before the lookup so the returned ResolutionDetails includes the correct variant identifier. Added test: TargetingReturnsNull_FallsBackToDefaultVariant Signed-off-by: Eden Yefet <edenyefet@gmail.com>
|
Hey @kylejuliandev, is there an ETA for the review and merge of this PR? Is there anything else that is required from my part? |
Hey @aedeny! Apologies, I have been away. I will resume my review this week 😄 |
| } | ||
|
|
||
| public async ValueTask DisposeAsync() | ||
| { |
There was a problem hiding this comment.
suggestion: is it worth adding a private bool field (e.g. _disposed) which we can check for redundant dispose calls?
| _debounceTimer?.Dispose(); | ||
| _evaluatorLock.Dispose(); | ||
| _cts.Dispose(); |
There was a problem hiding this comment.
suggestion: If the timer fires while LoadFlags() is executing (holding the write lock), Shutdown() calling _evaluatorLock.Dispose() will throw SynchronizationLockException. Would it be safer to cancel the CTS first, then dispose the watcher, then dispose the lock (after draining any in-flight callbacks).
| ResolverType.RPC => 8013, | ||
| ResolverType.IN_PROCESS => 8015, | ||
| _ => throw new NotImplementedException("ResolverType does not use Ports.") | ||
| _ => 8013 |
There was a problem hiding this comment.
thought: as you return early above I think you can leave the NotImplementedException in here
|
|
||
| var fileWatcher = new FileSystemHashWatcher(_filePath, _logger, _fileChangePollingInterval); | ||
|
|
||
| fileWatcher.FileChanged += (sender, e) => |
There was a problem hiding this comment.
thought: we subscribe to FileChanged, does this included when a file is deleted?
| FileOptions.SequentialScan)) | ||
| { | ||
| var hash = _murmur128.ComputeHash(fs); | ||
| return (hash, (int)fs.Length); |
There was a problem hiding this comment.
thought: does the int cast overflow if the file is too large?
5 participants
Add support for reading flagd feature flag definitions directly from a local file, with automatic change detection and live reload.
New resolver:
FileBasedResolver: reads a flagd flag definition JSON file from disk, validates it against the JSON schema, and evaluates flags locally via JsonEvaluator. Supports waiting for the file to appear (configurable timeout, default 5 min) and reloading on changes.
FileSystemHashWatcher: polls file content using MurmurHash-128 to detect changes. Designed for environments where filesystem events are unreliable (e.g., NFS, container-mounted volumes). Configurable polling interval (default 1 min). Baseline hash is computed synchronously on Start() to prevent race conditions.
When hash-based detection is not enabled, a standard FileSystemWatcher with 500ms debounce is used instead.
Configuration via FLAGD_RESOLVER=file, FLAGD_SOURCE_FILE_PATH, and FLAGD_HASH_FILE_CHANGE env vars. Builder API: WithSourceFilePath(), WithUseHashFileChangeDetection(). DI options: FlagdProviderOptions.SourceFilePath, FlagdProviderOptions.UseHashFileChangeDetection.
Error handling: FileShare.ReadWrite|Delete for safe concurrent access, schema validation on Init, ProviderError events on parse failures without crashing, ReaderWriterLockSlim protects evaluator during reloads.
Tests: 31 new tests (201 total pass) covering FileBasedResolver lifecycle and resolution, FileSystemHashWatcher polling, FlagdConfig env var parsing, FlagdProvider instantiation, and DI option mapping.
Documentation: Updated README with FILE resolver config table entries, usage examples, and hash-based detection docs.
This PR
Related Issues
Fixes #1234523
Notes
Follow-up Tasks
How to test