Commit 6ec3953e authored by Stan Hu's avatar Stan Hu

Reduce queries needed to check if node is a primary or secondary Geo node

If the Geo check for `secondary?` failed, the `RequestStore` cache would always
be set to `false` but the cache would not actually be used due to the semantics
of the ||= operator. This led to dozens of SQL calls with:

```sql
SELECT  1 AS one FROM "geo_nodes" LIMIT 1
```

This commit fixes the caching to handle boolean values properly.

Closes #1644
parent edacfbf8
---
title: Reduce queries needed to check if node is a primary or secondary Geo node
merge_request:
author:
...@@ -19,7 +19,7 @@ module Gitlab ...@@ -19,7 +19,7 @@ module Gitlab
end end
def self.enabled? def self.enabled?
RequestStore.store[:geo_node_enabled] ||= GeoNode.exists? self.cache_boolean(:geo_node_enabled) { GeoNode.exists? }
end end
def self.license_allows? def self.license_allows?
...@@ -27,11 +27,11 @@ module Gitlab ...@@ -27,11 +27,11 @@ module Gitlab
end end
def self.primary? def self.primary?
RequestStore.store[:geo_node_primary?] ||= self.enabled? && self.current_node && self.current_node.primary? self.cache_boolean(:geo_node_primary) { self.enabled? && self.current_node && self.current_node.primary? }
end end
def self.secondary? def self.secondary?
RequestStore.store[:geo_node_secondary] ||= self.enabled? && self.current_node && !self.current_node.primary? self.cache_boolean(:geo_node_secondary) { self.enabled? && self.current_node && !self.current_node.primary? }
end end
def self.geo_node?(host:, port:) def self.geo_node?(host:, port:)
...@@ -52,5 +52,14 @@ module Gitlab ...@@ -52,5 +52,14 @@ module Gitlab
RequestStore.store[:geo_oauth_application] ||= RequestStore.store[:geo_oauth_application] ||=
Gitlab::Geo.current_node.oauth_application or raise OauthApplicationUndefinedError Gitlab::Geo.current_node.oauth_application or raise OauthApplicationUndefinedError
end end
def self.cache_boolean(key, &block)
return yield unless RequestStore.active?
val = RequestStore.store[key]
return val unless val.nil?
RequestStore[key] = yield
end
end end
end end
...@@ -37,6 +37,23 @@ describe Gitlab::Geo, lib: true do ...@@ -37,6 +37,23 @@ describe Gitlab::Geo, lib: true do
expect(described_class.enabled?).to be_falsey expect(described_class.enabled?).to be_falsey
end end
end end
context 'with RequestStore enabled' do
before do
RequestStore.begin!
end
after do
RequestStore.end!
RequestStore.clear!
end
it 'return false when no GeoNode exists' do
expect(GeoNode).to receive(:exists?).once.and_call_original
2.times { expect(described_class.enabled?).to be_falsey }
end
end
end end
describe 'readonly?' do describe 'readonly?' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment