From 4d3da70bc6a733b660bd2247666e9703848d38e4 Mon Sep 17 00:00:00 2001 From: Rafael dos Santos Silva Date: Tue, 15 Feb 2022 10:36:07 -0300 Subject: [PATCH] PERF: Use Redis `SET EX GET` instead of LUA script for counting (#15939) This will prevent Discourse from booting on Redis < 6.2.0 --- app/models/concerns/cached_counting.rb | 11 +---------- config/initializers/001-redis.rb | 5 +++++ spec/models/application_request_spec.rb | 4 ++-- spec/models/web_crawler_request_spec.rb | 4 ++-- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/app/models/concerns/cached_counting.rb b/app/models/concerns/cached_counting.rb index 86871677c81..dfa5a8ec645 100644 --- a/app/models/concerns/cached_counting.rb +++ b/app/models/concerns/cached_counting.rb @@ -42,19 +42,10 @@ module CachedCounting raise NotImplementedError end - GET_AND_RESET = <<~LUA - local val = redis.call('get', KEYS[1]) - redis.call('set', KEYS[1], '0') - return val - LUA - # this may seem a bit fancy but in so it allows # for concurrent calls without double counting def get_and_reset(key) - namespaced_key = Discourse.redis.namespace_key(key) - val = Discourse.redis.without_namespace.eval(GET_AND_RESET, keys: [namespaced_key]).to_i - Discourse.redis.expire(key, 259200) # SET removes expiry, so set it again - val + Discourse.redis.set(key, '0', ex: 259200, get: true).to_i end def request_id(query_params, retries = 0) diff --git a/config/initializers/001-redis.rb b/config/initializers/001-redis.rb index 3957d626b29..574eb092ea8 100644 --- a/config/initializers/001-redis.rb +++ b/config/initializers/001-redis.rb @@ -8,3 +8,8 @@ end # Pending https://github.com/MiniProfiler/rack-mini-profiler/pull/450 and # upgrade to Sidekiq 6.1 Redis.exists_returns_integer = true + +if Gem::Version.new(Discourse.redis.info['redis_version']) < Gem::Version.new("6.2.0") + STDERR.puts "Discourse requires Redis 6.2.0 or up" + exit 1 +end diff --git a/spec/models/application_request_spec.rb b/spec/models/application_request_spec.rb index 0531e1c8dd6..2212a1dd8d1 100644 --- a/spec/models/application_request_spec.rb +++ b/spec/models/application_request_spec.rb @@ -30,14 +30,14 @@ describe ApplicationRequest do inc(:http_total) Discourse.redis.without_namespace.stubs(:incr).raises(Redis::CommandError.new("READONLY")) - Discourse.redis.without_namespace.stubs(:eval).raises(Redis::CommandError.new("READONLY")) + Discourse.redis.without_namespace.stubs(:set).raises(Redis::CommandError.new("READONLY")) # flush will be deferred no error raised inc(:http_total, autoflush: 3) ApplicationRequest.write_cache! Discourse.redis.without_namespace.unstub(:incr) - Discourse.redis.without_namespace.unstub(:eval) + Discourse.redis.without_namespace.unstub(:set) inc(:http_total, autoflush: 3) expect(ApplicationRequest.http_total.first.count).to eq(3) diff --git a/spec/models/web_crawler_request_spec.rb b/spec/models/web_crawler_request_spec.rb index 6e025e500a7..9bb42b69d8d 100644 --- a/spec/models/web_crawler_request_spec.rb +++ b/spec/models/web_crawler_request_spec.rb @@ -32,13 +32,13 @@ describe WebCrawlerRequest do inc('Googlebot') Discourse.redis.without_namespace.stubs(:incr).raises(Redis::CommandError.new("READONLY")) - Discourse.redis.without_namespace.stubs(:eval).raises(Redis::CommandError.new("READONLY")) + Discourse.redis.without_namespace.stubs(:set).raises(Redis::CommandError.new("READONLY")) inc('Googlebot', autoflush: 3) WebCrawlerRequest.write_cache! Discourse.redis.without_namespace.unstub(:incr) - Discourse.redis.without_namespace.unstub(:eval) + Discourse.redis.without_namespace.unstub(:set) inc('Googlebot', autoflush: 3) expect(web_crawler_request('Googlebot').count).to eq(3)