Merge branch '33279_pc_application_settings_performance' into 'master'

redesign caching of application settings

See merge request !11894

4 files changed, 43 insertions, 25 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 3b49cb4e3bf..2192f76499d 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -189,8 +189,9 @@ class ApplicationSetting < ActiveRecord::Base
   end
 
   def self.cached
-    ensure_cache_setup
-    Rails.cache.fetch(CACHE_KEY)
+    value = Rails.cache.read(CACHE_KEY)
+    ensure_cache_setup if value.present?
+    value
   end
 
   def self.ensure_cache_setup
diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb
index 9e14b35b0f8..48735fd197d 100644
--- a/lib/gitlab/current_settings.rb
+++ b/lib/gitlab/current_settings.rb
@@ -8,39 +8,55 @@ module Gitlab
       end
     end
 
-    def ensure_application_settings!
-      return fake_application_settings unless connect_to_db?
+    delegate :sidekiq_throttling_enabled?, to: :current_application_settings
 
-      unless ENV['IN_MEMORY_APPLICATION_SETTINGS'] == 'true'
-        begin
-          settings = ::ApplicationSetting.current
-        # In case Redis isn't running or the Redis UNIX socket file is not available
-        rescue ::Redis::BaseError, ::Errno::ENOENT
-          settings = ::ApplicationSetting.last
-        end
+    def fake_application_settings
+      OpenStruct.new(::ApplicationSetting.defaults)
+    end
 
-        settings ||= ::ApplicationSetting.create_from_defaults
+    private
+
+    def ensure_application_settings!
+      unless ENV['IN_MEMORY_APPLICATION_SETTINGS'] == 'true'
+        settings = retrieve_settings_from_database?
       end
 
       settings || in_memory_application_settings
     end
 
-    delegate :sidekiq_throttling_enabled?, to: :current_application_settings
+    def retrieve_settings_from_database?
+      settings = retrieve_settings_from_database_cache?
+      return settings if settings.present?
+
+      return fake_application_settings unless connect_to_db?
+
+      begin
+        db_settings = ::ApplicationSetting.current
+        # In case Redis isn't running or the Redis UNIX socket file is not available
+      rescue ::Redis::BaseError, ::Errno::ENOENT
+        db_settings = ::ApplicationSetting.last
+      end
+      db_settings || ::ApplicationSetting.create_from_defaults
+    end
+
+    def retrieve_settings_from_database_cache?
+      begin
+        settings = ApplicationSetting.cached
+      rescue ::Redis::BaseError, ::Errno::ENOENT
+        # In case Redis isn't running or the Redis UNIX socket file is not available
+        settings = nil
+      end
+      settings
+    end
 
     def in_memory_application_settings
       @in_memory_application_settings ||= ::ApplicationSetting.new(::ApplicationSetting.defaults)
-    # In case migrations the application_settings table is not created yet,
-    # we fallback to a simple OpenStruct
     rescue ActiveRecord::StatementInvalid, ActiveRecord::UnknownAttributeError
+      # In case migrations the application_settings table is not created yet,
+      # we fallback to a simple OpenStruct
       fake_application_settings
     end
 
-    def fake_application_settings
-      OpenStruct.new(::ApplicationSetting.defaults)
-    end
-
-    private
-
     def connect_to_db?
       # When the DBMS is not available, an exception (e.g. PG::ConnectionBad) is raised
       active_db_connection = ActiveRecord::Base.connection.active? rescue false
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index a25db7a65fb..08024a2148b 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -126,7 +126,7 @@ describe Projects::MergeRequestsController do
 
           recorded = ActiveRecord::QueryRecorder.new { go(format: :json) }
 
-          expect(recorded.count).to be_within(5).of(59)
+          expect(recorded.count).to be_within(5).of(50)
           expect(recorded.cached_count).to eq(0)
         end
       end
diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb
index c796c98ec9f..fda39d78610 100644
--- a/spec/lib/gitlab/current_settings_spec.rb
+++ b/spec/lib/gitlab/current_settings_spec.rb
@@ -14,20 +14,20 @@ describe Gitlab::CurrentSettings do
       end
 
       it 'attempts to use cached values first' do
-        expect(ApplicationSetting).to receive(:current)
-        expect(ApplicationSetting).not_to receive(:last)
+        expect(ApplicationSetting).to receive(:cached)
 
         expect(current_application_settings).to be_a(ApplicationSetting)
       end
 
       it 'falls back to DB if Redis returns an empty value' do
+        expect(ApplicationSetting).to receive(:cached).and_return(nil)
         expect(ApplicationSetting).to receive(:last).and_call_original
 
         expect(current_application_settings).to be_a(ApplicationSetting)
       end
 
       it 'falls back to DB if Redis fails' do
-        expect(ApplicationSetting).to receive(:current).and_raise(::Redis::BaseError)
+        expect(ApplicationSetting).to receive(:cached).and_raise(::Redis::BaseError)
         expect(ApplicationSetting).to receive(:last).and_call_original
 
         expect(current_application_settings).to be_a(ApplicationSetting)
@@ -37,6 +37,7 @@ describe Gitlab::CurrentSettings do
     context 'with DB unavailable' do
       before do
         allow_any_instance_of(described_class).to receive(:connect_to_db?).and_return(false)
+        allow_any_instance_of(described_class).to receive(:retrieve_settings_from_database_cache?).and_return(nil)
       end
 
       it 'returns an in-memory ApplicationSetting object' do