diff --git a/lib/discourse_redis.rb b/lib/discourse_redis.rb index f3f4993c819..9ed43c4e3a9 100644 --- a/lib/discourse_redis.rb +++ b/lib/discourse_redis.rb @@ -14,18 +14,20 @@ class DiscourseRedis @running = false @mutex = Mutex.new @slave_config = DiscourseRedis.slave_config + @timer_task = init_timer_task end def verify_master synchronize do - return if @running || recently_checked? - @running = true + return if @timer_task.running? end - Thread.new { initiate_fallback_to_master } + @timer_task.execute end def initiate_fallback_to_master + success = false + begin slave_client = ::Redis::Client.new(@slave_config) logger.info "#{log_prefix}: Checking connection to master server..." @@ -39,13 +41,14 @@ class DiscourseRedis Discourse.clear_readonly! Discourse.request_refresh! - @master = true + self.master = true + success = true end ensure - @running = false - @last_checked = Time.zone.now slave_client.disconnect end + + success end def master @@ -56,23 +59,18 @@ class DiscourseRedis synchronize { @master = args } end - def recently_checked? - if @last_checked - Time.zone.now <= (@last_checked + 5.seconds) - else - false - end - end - - # Used for testing - def reset! - @master = true - @last_checked = nil - @running = false + def running? + synchronize { @timer_task.running? } end private + def init_timer_task + Concurrent::TimerTask.new(execution_interval: 10) do |task| + task.shutdown if initiate_fallback_to_master + end + end + def synchronize @mutex.synchronize { yield } end @@ -87,9 +85,6 @@ class DiscourseRedis end class Connector < Redis::Client::Connector - MASTER = 'master'.freeze - SLAVE = 'slave'.freeze - def initialize(options) super(options) @slave_options = DiscourseRedis.slave_config(options) @@ -97,21 +92,18 @@ class DiscourseRedis end def resolve - - return @options unless @slave_options[:host] + return @slave_options if !@fallback_handler.master begin options = @options.dup options.delete(:connector) - client = ::Redis::Client.new(options) - client.call([:role])[0] - @options + client = Redis::Client.new(options) + loading = client.call([:info]).split("\r\n").include?("loading:1") + loading ? @slave_options : @options rescue Redis::ConnectionError, Redis::CannotConnectError, RuntimeError => ex - # A consul service name may be deregistered for a redis container setup raise ex if ex.class == RuntimeError && ex.message != "Name or service not known" - - return @slave_options if !@fallback_handler.master @fallback_handler.master = false + @fallback_handler.verify_master unless @fallback_handler.running? raise ex ensure client.disconnect diff --git a/spec/components/discourse_redis_spec.rb b/spec/components/discourse_redis_spec.rb index 0535d8fc376..8cc8c76d47f 100644 --- a/spec/components/discourse_redis_spec.rb +++ b/spec/components/discourse_redis_spec.rb @@ -42,7 +42,7 @@ describe DiscourseRedis do it 'should return the slave config when master is down' do begin - Redis::Client.any_instance.expects(:call).raises(Redis::CannotConnectError).twice + Redis::Client.any_instance.expects(:call).raises(Redis::CannotConnectError).once expect { connector.resolve }.to raise_error(Redis::CannotConnectError) config = connector.resolve @@ -58,7 +58,7 @@ describe DiscourseRedis do begin error = RuntimeError.new('Name or service not known') - Redis::Client.any_instance.expects(:call).raises(error).twice + Redis::Client.any_instance.expects(:call).raises(error).once expect { connector.resolve }.to raise_error(error) config = connector.resolve @@ -70,6 +70,18 @@ describe DiscourseRedis do end end + it "should return the slave config when master is still loading data" do + begin + Redis::Client.any_instance.expects(:call).with([:info]).returns("someconfig:haha\r\nloading:1") + config = connector.resolve + + expect(config[:host]).to eq(slave_host) + expect(config[:port]).to eq(slave_port) + ensure + fallback_handler.master = true + end + end + it "should raise the right error" do error = RuntimeError.new('test error') Redis::Client.any_instance.expects(:call).raises(error).twice @@ -79,34 +91,27 @@ describe DiscourseRedis do describe DiscourseRedis::FallbackHandler do after do - fallback_handler.reset! + fallback_handler.master = true end describe '#initiate_fallback_to_master' do - it 'should fallback to the master server once it is up' do - begin - fallback_handler.master = false - Redis::Client.any_instance.expects(:call).with([:info]).returns(DiscourseRedis::FallbackHandler::MASTER_LINK_STATUS) - - DiscourseRedis::FallbackHandler::CONNECTION_TYPES.each do |connection_type| - Redis::Client.any_instance.expects(:call).with([:client, [:kill, 'type', connection_type]]) - end - - fallback_handler.initiate_fallback_to_master - - expect(fallback_handler.master).to eq(true) - expect(Discourse.recently_readonly?).to eq(false) - ensure - fallback_handler.master = true - end + it 'should return the right value if the master server is still down' do + fallback_handler.master = false + Redis::Client.any_instance.expects(:call).with([:info]).returns("Some other stuff") + expect(fallback_handler.initiate_fallback_to_master).to eq(false) end - it "should restrict the number of checks" do - expect { fallback_handler.verify_master }.to change { Thread.list.count }.by(1) - expect(fallback_handler.master).to eq(true) - + it 'should fallback to the master server once it is up' do fallback_handler.master = false - expect { fallback_handler.verify_master }.to_not change { Thread.list.count } + Redis::Client.any_instance.expects(:call).with([:info]).returns(DiscourseRedis::FallbackHandler::MASTER_LINK_STATUS) + + DiscourseRedis::FallbackHandler::CONNECTION_TYPES.each do |connection_type| + Redis::Client.any_instance.expects(:call).with([:client, [:kill, 'type', connection_type]]) + end + + expect(fallback_handler.initiate_fallback_to_master).to eq(true) + expect(fallback_handler.master).to eq(true) + expect(Discourse.recently_readonly?).to eq(false) end end end