-
Notifications
You must be signed in to change notification settings - Fork 55
chore: Create FDv1 datasystem implementation #339
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?
Conversation
| # @return [Concurrent::Event] Event that will be set when initialization is complete | ||
| # | ||
| def start(ready_event) | ||
| def start |
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 was a little torn on having the ready_event passed in vs returning it. The existing data_source public interface for its start method returns a Concurrent::Event. This means we would either need an interface layer to sync the two events between the data system and the data source, or we would need a breaking change to have the data source interface accept the event vs return it.
|
|
||
| if @update_processor && @update_processor.initialized? | ||
| return DataAvailability::REFRESHED | ||
| end |
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.
Bug: LDD mode incorrectly reports data availability as refreshed
In LDD (LaunchDarkly Daemon) mode, data_availability returns REFRESHED even when the data store is empty. After start() is called, the NullUpdateProcessor.initialized? check always passes (since it unconditionally returns true), causing REFRESHED to be returned without verifying the store has any data. In LDD mode, data comes from an external daemon populating the store, so availability should be determined by the store's initialization state, not the null processor's state. This could mislead consumers into thinking fresh data is available when the store is actually empty.
Additional Locations (1)
| # | ||
| def set_flag_value_eval_fn(eval_fn) | ||
| @flag_tracker_impl = LaunchDarkly::Impl::FlagTracker.new(@flag_change_broadcaster, eval_fn) | ||
| end |
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.
Bug: Flag tracker replacement loses references to original instance
The set_flag_value_eval_fn method replaces @flag_tracker_impl with an entirely new FlagTracker instance. If any code calls flag_tracker before this method runs and caches the returned reference, that cached reference will continue pointing to the old tracker with the dummy evaluation function (lambda { |_key, _context| nil }). Flag value change listeners added via the stale reference would use the wrong eval function, always computing flag values as nil and failing to detect actual value changes. A better approach would be to update the eval function on the existing tracker instance rather than replacing it.
Note
Introduces
FDv1data system wiring to existing v1 processors, addsNullUpdateProcessor, updates theDataSystemAPI, integrates withLDClient, and adds comprehensive specs.LaunchDarkly::Impl::DataSystem::FDv1that wires v1 data source/store behind theDataSysteminterface.diagnostic_accumulatorto streaming, and manages broadcasters, status providers, and flag tracking.data_availabilityandtarget_availabilitybased on config, processor, and store state.LaunchDarkly::Impl::DataSource::NullUpdateProcessorthat instantly marks initialized and no-ops on stop.startto take no args and return aConcurrent::Event.set_diagnostic_accumulatorhook.DataAvailability(REFRESHED,ALL,.at_least?).DiagnosticAccumulator,DiagnosticSource) and protocol mixins (Initializer,Synchronizer), plusUpdatevalue object.NullUpdateProcessor; removes internalNullUpdateProcessorclass.spec/impl/datasystem/fdv1_spec.rbcovering processor selection, diagnostics, availability, and lifecycle.spec/impl/datasystem_spec.rbfor new API and mixins; enhancesspec/mock_components.rbwithMockUpdateProcessorand namespaced null processor.Written by Cursor Bugbot for commit 66b8876. This will update automatically on new commits. Configure here.