-
-
Notifications
You must be signed in to change notification settings - Fork 418
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
Poll Memoizer #704
base: main
Are you sure you want to change the base?
Poll Memoizer #704
Conversation
@adapter = if memoize == :poll | ||
Adapters::Poll.new(Adapters::Memory.new, adapter, | ||
key: 'memoizer', | ||
interval: 5, |
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.
FIXME: make configurable
@adapter = adapter | ||
@adapter = if memoize == :poll | ||
Adapters::Poll.new(Adapters::Memory.new, adapter, | ||
key: 'memoizer', |
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.
If multiple DSL
instances are created with memoize: :poll
and different adapters, this will not behave as expected. I wonder if Poller.get
should also look for equality in the adapters. 🤔
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.
Shouldn't the key just be unique per adapter?
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.
Or rather a better way to phrase that, is there a way to make the key unique per instance? We have the name. That would help with different adapters. Perhaps there is another way to add uniqueness per instance too... hmm
instrumenter: @instrumenter | ||
).tap do |poll| | ||
# Force poller to sync in current thread now | ||
poll.poller.sync |
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'd be tempted to make a method for this to avoid reaching into poller.
I'm going to hold off on this for now since we're not using this adapter in production. Eventually I do think some kind of interval-based memoization (instead of request-based) will really help improve performance, but I'd like to think on it a bit. |
* origin/main: (454 commits) Use flippercloud.io API, don't retry failures Move rails constants Show deprecation warning for Rails < 6.1.0 Show deprecation warning for Ruby < 3.0 UI: show a badge if a new release is available Use GitHub Releases for changelog Fix warning from script/bootstrap Prepare for 1.1.2 release Update Changelog Fix remove_index for older Rails versions Add length to update migration Create instead of purge before all Indifferent access for old Rails versions Fix rescue that may be obscuring errors on Actions Skip AR tests locally if db can't connect Add rails to readme like summary Add feature flag to summaries Duplicate postgres env config Move postgres env config Configure postgres and mysql on CI ...
Currently, Flipper is configured by default to perform memoization and preloading per request, which will query the primary storage adapter each time a request is received. This is fine on a lot of apps, but can add some overhead on apps with heavy traffic.
This pull request adds a
memoize = :poll
configuration option:Setting
memoize = :poll
will switch memoization to a new background thread polling strategy based on thePoll
adapter introduced in #682. ThePoll
adapter will start a background thread that will load all features from the primary storage adapter into aMemory
adapter on an interval. Flipper will then read from thisMemory
adapter instead of the primary storage adapter (it actually has multiple instances of theMemory
adapter, one for the background, and then one for each app thread that is synchronized before each request).TODO: