From b4936f62afd404a89b277cd8982feb007f3b6c44 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 8 Jun 2016 12:18:37 +0100 Subject: [PATCH] Check that return values from different redis backends match --- .../app/coffee/RedisBackend.coffee | 115 ++++--- .../RedisBackend/RedisBackendTests.coffee | 307 ++++++++++++++++++ 2 files changed, 383 insertions(+), 39 deletions(-) create mode 100644 services/document-updater/test/unit/coffee/RedisBackend/RedisBackendTests.coffee diff --git a/services/document-updater/app/coffee/RedisBackend.coffee b/services/document-updater/app/coffee/RedisBackend.coffee index e242beaec2..125aa29d03 100644 --- a/services/document-updater/app/coffee/RedisBackend.coffee +++ b/services/document-updater/app/coffee/RedisBackend.coffee @@ -1,5 +1,7 @@ Settings = require "settings-sharelatex" async = require "async" +_ = require "underscore" +logger = require "logger-sharelatex" class Client constructor: (@clients) -> @@ -10,6 +12,7 @@ class Client rclient: client.rclient.multi() key_schema: client.key_schema primary: client.primary + driver: client.driver } ) @@ -19,56 +22,84 @@ class MultiClient exec: (callback) -> jobs = @clients.map (client) -> (cb) -> - console.error "EXEC", client.rclient.queue - client.rclient.exec (result...) -> - console.error "EXEC RESULT", result + client.rclient.exec (error, result) -> + if client.driver == "ioredis" + # ioredis returns an results like: + # [ [null, 42], [null, "foo"] ] + # where the first entries in each 2-tuple are + # presumably errors for each individual command, + # and the second entry is the result. We need to transform + # this into the same result as the old redis driver: + # [ 42, "foo" ] + filtered_result = [] + for entry in result or [] + if entry[0]? + return cb(entry[0]) + else + filtered_result.push entry[1] + result = filtered_result + if client.primary # Return this result as the actual result - callback(result...) + callback(error, result) # Send the rest through for comparison - cb(result...) + cb(error, result) async.parallel jobs, (error, results) -> - console.error "EXEC RESULTS", results + if error? + logger.error {err: error}, "error in redis backend" + else + compareResults(results) -COMMANDS = [ - "get", "smembers", "set", "srem", "sadd", "del", "lrange", - "llen", "rpush", "expire", "ltrim", "incr" -] -for command in COMMANDS - do (command) -> - Client.prototype[command] = (key_builder, args..., callback) -> - async.parallel @clients.map (client) -> +COMMANDS = { + "get": 0, + "smembers": 0, + "set": 0, + "srem": 0, + "sadd": 0, + "del": 0, + "lrange": 0, + "llen": 0, + "rpush": 0, + "expire": 0, + "ltrim": 0, + "incr": 0, + "eval": 2 +} +for command, key_pos of COMMANDS + do (command, key_pos) -> + Client.prototype[command] = (args..., callback) -> + jobs = @clients.map (client) -> (cb) -> + key_builder = args[key_pos] key = key_builder(client.key_schema) - console.error "COMMAND", command, key, args - client.rclient[command] key, args..., (result...) -> - console.log "RESULT", command, result + args_with_key = args.slice(0) + args_with_key[key_pos] = key + client.rclient[command] args_with_key..., (error, result...) -> if client.primary # Return this result as the actual result - callback?(result...) + callback(error, result...) # Send the rest through for comparison - cb(result...) - , (error, results) -> - console.log "#{command} RESULTS", results + cb(error, result...) + async.parallel jobs, (error, results) -> + if error? + logger.error {err: error}, "error in redis backend" + else + compareResults(results) - MultiClient.prototype[command] = (key_builder, args...) -> + MultiClient.prototype[command] = (args...) -> for client in @clients + key_builder = args[key_pos] key = key_builder(client.key_schema) - console.error "MULTI COMMAND", command, key, args + args_with_key = args.slice(0) + args_with_key[key_pos] = key client.rclient[command] key, args... -Client::eval = (script, pos, key_builder, args..., callback) -> - async.parallel @clients.map (client) -> - (cb) -> - key = key_builder(client.key_schema) - client.rclient.eval script, pos, key, args..., (result...) -> - if client.primary - # Return this result as the actual result - callback(result...) - # Send the rest through for comparison - cb(result...) - , (error, results) -> - console.log "#{command} RESULTS", results +compareResults = (results) -> + return if results.length < 2 + first = results[0] + for result in results.slice(1) + if not _.isEqual(first, result) + logger.warn { results }, "redis return values do not match" module.exports = createClient: () -> @@ -80,9 +111,15 @@ module.exports = if config.cluster? Redis = require("ioredis") rclient = new Redis.Cluster(config.cluster) + driver = "ioredis" else - rclient = require("redis-sharelatex").createClient(config) - rclient: rclient - key_schema: config.key_schema - primary: config.primary + {host, port, password} = config + rclient = require("redis-sharelatex").createClient({host, port, password}) + driver = "redis" + return { + rclient: rclient + key_schema: config.key_schema + primary: config.primary + driver: driver + } return new Client(clients) \ No newline at end of file diff --git a/services/document-updater/test/unit/coffee/RedisBackend/RedisBackendTests.coffee b/services/document-updater/test/unit/coffee/RedisBackend/RedisBackendTests.coffee new file mode 100644 index 0000000000..ca48aff7ff --- /dev/null +++ b/services/document-updater/test/unit/coffee/RedisBackend/RedisBackendTests.coffee @@ -0,0 +1,307 @@ +sinon = require('sinon') +chai = require('chai') +should = chai.should() +modulePath = "../../../../app/js/RedisBackend.js" +SandboxedModule = require('sandboxed-module') +RedisKeyBuilder = require "../../../../app/js/RedisKeyBuilder" + +describe "RedisBackend", -> + beforeEach -> + @Settings = + redis: + documentupdater: [{ + primary: true + port: "6379" + host: "localhost" + password: "single-password" + key_schema: + blockingKey: ({doc_id}) -> "Blocking:#{doc_id}" + docLines: ({doc_id}) -> "doclines:#{doc_id}" + docOps: ({doc_id}) -> "DocOps:#{doc_id}" + docVersion: ({doc_id}) -> "DocVersion:#{doc_id}" + projectKey: ({doc_id}) -> "ProjectId:#{doc_id}" + pendingUpdates: ({doc_id}) -> "PendingUpdates:#{doc_id}" + docsInProject: ({project_id}) -> "DocsIn:#{project_id}" + }, { + cluster: [{ + port: "7000" + host: "localhost" + }] + password: "cluster-password" + key_schema: + blockingKey: ({doc_id}) -> "Blocking:{#{doc_id}}" + docLines: ({doc_id}) -> "doclines:{#{doc_id}}" + docOps: ({doc_id}) -> "DocOps:{#{doc_id}}" + docVersion: ({doc_id}) -> "DocVersion:{#{doc_id}}" + projectKey: ({doc_id}) -> "ProjectId:{#{doc_id}}" + pendingUpdates: ({doc_id}) -> "PendingUpdates:{#{doc_id}}" + docsInProject: ({project_id}) -> "DocsIn:{#{project_id}}" + }] + + test_context = @ + class Cluster + constructor: (@config) -> + test_context.rclient_ioredis = @ + + @RedisBackend = SandboxedModule.require modulePath, requires: + "settings-sharelatex": @Settings + "logger-sharelatex": @logger = { error: sinon.stub(), log: sinon.stub(), warn: sinon.stub() } + "redis-sharelatex": @redis = + createClient: sinon.stub().returns @rclient_redis = {} + "ioredis": @ioredis = + Cluster: Cluster + @client = @RedisBackend.createClient() + + @doc_id = "mock-doc-id" + + it "should create a redis client", -> + @redis.createClient + .calledWith({ + port: "6379" + host: "localhost" + password: "single-password" + }) + .should.equal true + + it "should create an ioredis cluster client", -> + @rclient_ioredis.config.should.deep.equal [{ + port: "7000" + host: "localhost" + }] + + describe "individual commands", -> + describe "with the same results", -> + beforeEach (done) -> + @content = "bar" + @rclient_redis.get = sinon.stub() + @rclient_redis.get.withArgs("doclines:#{@doc_id}").yields(null, @content) + @rclient_ioredis.get = sinon.stub() + @rclient_ioredis.get.withArgs("doclines:{#{@doc_id}}").yields(null, @content) + @client.get RedisKeyBuilder.docLines({doc_id: @doc_id}), (error, @result) => + setTimeout () -> # Let all background requests complete + done(error) + + it "should return the result", -> + @result.should.equal @content + + it "should have called the redis client with the appropriate key", -> + @rclient_redis.get + .calledWith("doclines:#{@doc_id}") + .should.equal true + + it "should have called the ioredis cluster client with the appropriate key", -> + @rclient_ioredis.get + .calledWith("doclines:{#{@doc_id}}") + .should.equal true + + describe "with different results", -> + beforeEach (done) -> + @rclient_redis.get = sinon.stub() + @rclient_redis.get.withArgs("doclines:#{@doc_id}").yields(null, "primary-result") + @rclient_ioredis.get = sinon.stub() + @rclient_ioredis.get.withArgs("doclines:{#{@doc_id}}").yields(null, "secondary-result") + @client.get RedisKeyBuilder.docLines({doc_id: @doc_id}), (error, @result) => + setTimeout () -> # Let all background requests complete + done(error) + + it "should return the primary result", -> + @result.should.equal "primary-result" + + it "should log out the difference", -> + @logger.warn + .calledWith({ + results: [ + "primary-result", + "secondary-result" + ] + }, "redis return values do not match") + .should.equal true + + describe "when the secondary errors", -> + beforeEach (done) -> + @rclient_redis.get = sinon.stub() + @rclient_redis.get.withArgs("doclines:#{@doc_id}").yields(null, "primary-result") + @rclient_ioredis.get = sinon.stub() + @rclient_ioredis.get.withArgs("doclines:{#{@doc_id}}").yields(@error = new Error("oops")) + @client.get RedisKeyBuilder.docLines({doc_id: @doc_id}), (error, @result) => + setTimeout () -> # Let all background requests complete + done(error) + + it "should return the primary result", -> + @result.should.equal "primary-result" + + it "should log out the secondary error", -> + @logger.error + .calledWith({ + err: @error + }, "error in redis backend") + .should.equal true + + describe "when the primary errors", -> + beforeEach (done) -> + @rclient_redis.get = sinon.stub() + @rclient_redis.get.withArgs("doclines:#{@doc_id}").yields(@error = new Error("oops")) + @rclient_ioredis.get = sinon.stub() + @rclient_ioredis.get.withArgs("doclines:{#{@doc_id}}").yields(null, "secondary-result") + @client.get RedisKeyBuilder.docLines({doc_id: @doc_id}), (@returned_error, @result) => + setTimeout () -> # Let all background requests complete + done() + + it "should return the error", -> + @returned_error.should.equal @error + + it "should log out the error", -> + @logger.error + .calledWith({ + err: @error + }, "error in redis backend") + .should.equal true + + describe "when the command has the key in a non-zero argument index", -> + beforeEach (done) -> + @script = "mock-script" + @key_count = 1 + @value = "mock-value" + @rclient_redis.eval = sinon.stub() + @rclient_redis.eval.withArgs(@script, @key_count, "Blocking:#{@doc_id}", @value).yields(null) + @rclient_ioredis.eval = sinon.stub() + @rclient_ioredis.eval.withArgs(@script, @key_count, "Blocking:{#{@doc_id}}", @value).yields(null, @content) + @client.eval @script, @key_count, RedisKeyBuilder.blockingKey({doc_id: @doc_id}), @value, (error) => + setTimeout () -> # Let all background requests complete + done(error) + + it "should have called the redis client with the appropriate key", -> + @rclient_redis.eval + .calledWith(@script, @key_count, "Blocking:#{@doc_id}", @value) + .should.equal true + + it "should have called the ioredis cluster client with the appropriate key", -> + @rclient_ioredis.eval + .calledWith(@script, @key_count, "Blocking:{#{@doc_id}}", @value) + .should.equal true + + describe "multi commands", -> + beforeEach -> + # We will test with: + # rclient.multi() + # .get("doclines:foo") + # .get("DocVersion:foo") + # .exec (...) -> + @doclines = "mock-doclines" + @version = "42" + @rclient_redis.multi = sinon.stub().returns @rclient_redis + @rclient_ioredis.multi = sinon.stub().returns @rclient_ioredis + + describe "with the same results", -> + beforeEach (done) -> + @rclient_redis.get = sinon.stub() + @rclient_redis.exec = sinon.stub().yields(null, [@doclines, @version]) + @rclient_ioredis.get = sinon.stub() + @rclient_ioredis.exec = sinon.stub().yields(null, [ [null, @doclines], [null, @version] ]) + + multi = @client.multi() + multi.get RedisKeyBuilder.docLines({doc_id: @doc_id}) + multi.get RedisKeyBuilder.docVersion({doc_id: @doc_id}) + multi.exec (error, @result) => + setTimeout () -> + done(error) + + it "should return the result", -> + @result.should.deep.equal [@doclines, @version] + + it "should have called the redis client with the appropriate keys", -> + @rclient_redis.get + .calledWith("doclines:#{@doc_id}") + .should.equal true + @rclient_redis.get + .calledWith("DocVersion:#{@doc_id}") + .should.equal true + @rclient_ioredis.exec + .called + .should.equal true + + it "should have called the ioredis cluster client with the appropriate keys", -> + @rclient_ioredis.get + .calledWith("doclines:{#{@doc_id}}") + .should.equal true + @rclient_ioredis.get + .calledWith("DocVersion:{#{@doc_id}}") + .should.equal true + @rclient_ioredis.exec + .called + .should.equal true + + describe "with different results", -> + beforeEach (done) -> + @rclient_redis.get = sinon.stub() + @rclient_redis.exec = sinon.stub().yields(null, [@doclines, @version]) + @rclient_ioredis.get = sinon.stub() + @rclient_ioredis.exec = sinon.stub().yields(null, [ [null, "different-doc-lines"], [null, @version] ]) + + multi = @client.multi() + multi.get RedisKeyBuilder.docLines({doc_id: @doc_id}) + multi.get RedisKeyBuilder.docVersion({doc_id: @doc_id}) + multi.exec (error, @result) => + setTimeout () -> + done(error) + + it "should return the primary result", -> + @result.should.deep.equal [@doclines, @version] + + it "should log out the difference", -> + @logger.warn + .calledWith({ + results: [ + [@doclines, @version], + ["different-doc-lines", @version] + ] + }, "redis return values do not match") + .should.equal true + + describe "when the secondary errors", -> + beforeEach (done) -> + @rclient_redis.get = sinon.stub() + @rclient_redis.exec = sinon.stub().yields(null, [@doclines, @version]) + @rclient_ioredis.get = sinon.stub() + @rclient_ioredis.exec = sinon.stub().yields(@error = new Error("oops")) + + multi = @client.multi() + multi.get RedisKeyBuilder.docLines({doc_id: @doc_id}) + multi.get RedisKeyBuilder.docVersion({doc_id: @doc_id}) + multi.exec (error, @result) => + setTimeout () -> + done(error) + + it "should return the primary result", -> + @result.should.deep.equal [@doclines, @version] + + it "should log out the secondary error", -> + @logger.error + .calledWith({ + err: @error + }, "error in redis backend") + .should.equal true + + describe "when the secondary errors", -> + beforeEach (done) -> + @rclient_redis.get = sinon.stub() + @rclient_redis.exec = sinon.stub().yields(@error = new Error("oops")) + @rclient_ioredis.get = sinon.stub() + @rclient_ioredis.exec = sinon.stub().yields([ [null, @doclines], [null, @version] ]) + + multi = @client.multi() + multi.get RedisKeyBuilder.docLines({doc_id: @doc_id}) + multi.get RedisKeyBuilder.docVersion({doc_id: @doc_id}) + multi.exec (@returned_error) => + setTimeout () -> done() + + it "should return the error", -> + @returned_error.should.equal @error + + it "should log out the error", -> + @logger.error + .calledWith({ + err: @error + }, "error in redis backend") + .should.equal true +