Cache feature names in RequestStore
The GitHub importer (and probably other parts of our code) ends up calling Feature.persisted? many times (via Gitaly). By storing this data in RequestStore we can save ourselves _a lot_ of database queries. Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/39361
This commit is contained in:
parent
bda30182e0
commit
90be53c5d3
2 changed files with 54 additions and 1 deletions
|
@ -5,6 +5,10 @@ class Feature
|
|||
class FlipperFeature < Flipper::Adapters::ActiveRecord::Feature
|
||||
# Using `self.table_name` won't work. ActiveRecord bug?
|
||||
superclass.table_name = 'features'
|
||||
|
||||
def self.feature_names
|
||||
pluck(:key)
|
||||
end
|
||||
end
|
||||
|
||||
class FlipperGate < Flipper::Adapters::ActiveRecord::Gate
|
||||
|
@ -22,11 +26,19 @@ class Feature
|
|||
flipper.feature(key)
|
||||
end
|
||||
|
||||
def persisted_names
|
||||
if RequestStore.active?
|
||||
RequestStore[:flipper_persisted_names] ||= FlipperFeature.feature_names
|
||||
else
|
||||
FlipperFeature.feature_names
|
||||
end
|
||||
end
|
||||
|
||||
def persisted?(feature)
|
||||
# Flipper creates on-memory features when asked for a not-yet-created one.
|
||||
# If we want to check if a feature has been actually set, we look for it
|
||||
# on the persisted features list.
|
||||
all.map(&:name).include?(feature.name)
|
||||
persisted_names.include?(feature.name)
|
||||
end
|
||||
|
||||
def enabled?(key, thing = nil)
|
||||
|
|
|
@ -13,6 +13,47 @@ describe Feature do
|
|||
end
|
||||
end
|
||||
|
||||
describe '.persisted_names' do
|
||||
it 'returns the names of the persisted features' do
|
||||
Feature::FlipperFeature.create!(key: 'foo')
|
||||
|
||||
expect(described_class.persisted_names).to eq(%w[foo])
|
||||
end
|
||||
|
||||
it 'returns an empty Array when no features are presisted' do
|
||||
expect(described_class.persisted_names).to be_empty
|
||||
end
|
||||
|
||||
it 'caches the feature names when request store is active', :request_store do
|
||||
Feature::FlipperFeature.create!(key: 'foo')
|
||||
|
||||
expect(Feature::FlipperFeature)
|
||||
.to receive(:feature_names)
|
||||
.once
|
||||
.and_call_original
|
||||
|
||||
2.times do
|
||||
expect(described_class.persisted_names).to eq(%w[foo])
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe '.persisted?' do
|
||||
it 'returns true for a persisted feature' do
|
||||
Feature::FlipperFeature.create!(key: 'foo')
|
||||
|
||||
feature = double(:feature, name: 'foo')
|
||||
|
||||
expect(described_class.persisted?(feature)).to eq(true)
|
||||
end
|
||||
|
||||
it 'returns false for a feature that is not persisted' do
|
||||
feature = double(:feature, name: 'foo')
|
||||
|
||||
expect(described_class.persisted?(feature)).to eq(false)
|
||||
end
|
||||
end
|
||||
|
||||
describe '.all' do
|
||||
let(:features) { Set.new }
|
||||
|
||||
|
|
Loading…
Reference in a new issue