-
Notifications
You must be signed in to change notification settings - Fork 55
chore: Use FDv1 DataSystem in the ldclient #345
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 [Array<EvaluationDetail, [LaunchDarkly::Impl::Model::FeatureFlag, nil], [String, nil]>] | ||
| # | ||
| def variation_with_flag(key, context, default) | ||
| private def variation_with_flag(key, context, default) |
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.
The file used a mix of per method private and all methods after the declaration on line 717 of the original file. Moving to defining per method.
| # so we're not connecting to any LD services). | ||
| if !config.offline? && sdk_key.nil? | ||
| # SDK key can be nil only if using LDD or custom data source with events disabled | ||
| if config.send_events || (!config.use_ldd? && config.data_source.nil?) |
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.
This conditional doesn't make sense to me.
The SDK key is required if:
- We are sending events, or
- We aren't in daemon mode AND we don't have a data source?
If we aren't in daemon mode, then we should have a data source, and that's when an SDK key is required. If there isn't a data source, what is the key for?
| # | ||
| def initialized? | ||
| @config.offline? || @config.use_ldd? || @data_source.initialized? | ||
| @data_system.data_availability == @data_system.target_availability |
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.
Are we sure these lines are equivalent given our previous discussions about this? I can't remember exactly where we landed on that.
| # @return [LaunchDarkly::Interfaces::DataStore::StatusProvider] | ||
| # | ||
| attr_reader :data_store_status_provider | ||
| def data_store_status_provider |
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.
You could also make these delegators if you wanted to tighten the syntax up some. It would be something like:
require 'forwardable'
# ...
extend Forwardable
def_delegators :@data_system, :data_store_status_provider, :data_source_status_provider
Note
Replaces legacy data-source/store wiring in LDClient with FDv1 DataSystem and updates client to use its store, status providers, and broadcasters.
impl/data_system/fdv1.rb):config.feature_storeinFeatureStoreClientWrapper(avoid nested wrapping on postfork) and expose status providers/broadcasters/update sinks.start/stop), executor, and compute data availability/target; log when streaming is disabled (polling).ldclient.rb):FDv1for store access (@data_system.store), flag change broadcaster, and data/data-store status providers.@data_system; remove legacy data source plumbing.initialized?to comparedata_availabilityvstarget_availability.all_flags_stateto read from@data_system.store.spec/impl/data_system/fdv1_spec.rb):Written by Cursor Bugbot for commit ff845d5. This will update automatically on new commits. Configure here.