From cbdace73861234a432bca9891293bcf9b25ea955 Mon Sep 17 00:00:00 2001 From: Hayden Faulds Date: Fri, 29 Sep 2017 10:54:48 +0100 Subject: [PATCH] store pathname in Redis cache --- .../app/coffee/RedisManager.coffee | 13 +- .../config/settings.defaults.coffee | 1 + .../RedisManager/RedisManagerTests.coffee | 112 ++++++++++-------- 3 files changed, 74 insertions(+), 52 deletions(-) diff --git a/services/document-updater/app/coffee/RedisManager.coffee b/services/document-updater/app/coffee/RedisManager.coffee index b873f9bd6a..08f8e7595c 100644 --- a/services/document-updater/app/coffee/RedisManager.coffee +++ b/services/document-updater/app/coffee/RedisManager.coffee @@ -36,7 +36,7 @@ historyKeys = Settings.redis.history.key_schema module.exports = RedisManager = rclient: rclient - putDocInMemory : (project_id, doc_id, docLines, version, ranges, _callback)-> + putDocInMemory : (project_id, doc_id, docLines, version, ranges, pathname, _callback)-> timer = new metrics.Timer("redis.put-doc") callback = (error) -> timer.done() @@ -61,6 +61,7 @@ module.exports = RedisManager = multi.set keys.ranges(doc_id:doc_id), ranges else multi.del keys.ranges(doc_id:doc_id) + multi.set keys.pathname(doc_id:doc_id), pathname multi.exec (error, result) -> return callback(error) if error? # check the hash computed on the redis server @@ -86,6 +87,7 @@ module.exports = RedisManager = multi.del keys.docVersion(doc_id:doc_id) multi.del keys.docHash(doc_id:doc_id) multi.del keys.ranges(doc_id:doc_id) + multi.del keys.pathname(doc_id:doc_id) multi.del keys.unflushedTime(doc_id:doc_id) multi.exec (error) -> return callback(error) if error? @@ -106,7 +108,7 @@ module.exports = RedisManager = clearProjectState: (project_id, callback = (error) ->) -> rclient.del keys.projectState(project_id:project_id), callback - getDoc : (project_id, doc_id, callback = (error, lines, version, ranges, unflushedTime) ->)-> + getDoc : (project_id, doc_id, callback = (error, lines, version, ranges, pathname, unflushedTime) ->)-> timer = new metrics.Timer("redis.get-doc") multi = rclient.multi() multi.get keys.docLines(doc_id:doc_id) @@ -114,8 +116,9 @@ module.exports = RedisManager = multi.get keys.docHash(doc_id:doc_id) multi.get keys.projectKey(doc_id:doc_id) multi.get keys.ranges(doc_id:doc_id) + multi.get keys.pathname(doc_id:doc_id) multi.get keys.unflushedTime(doc_id:doc_id) - multi.exec (error, [docLines, version, storedHash, doc_project_id, ranges, unflushedTime])-> + multi.exec (error, [docLines, version, storedHash, doc_project_id, ranges, pathname, unflushedTime])-> timeSpan = timer.done() return callback(error) if error? # check if request took too long and bail out. only do this for @@ -144,14 +147,14 @@ module.exports = RedisManager = # doc is not in redis, bail out if !docLines? - return callback null, docLines, version, ranges + return callback null, docLines, version, ranges, pathname, unflushedTime # doc should be in project set, check if missing (workaround for missing docs from putDoc) rclient.sadd keys.docsInProject(project_id:project_id), doc_id, (error, result) -> return callback(error) if error? if result isnt 0 # doc should already be in set logger.error project_id: project_id, doc_id: doc_id, doc_project_id: doc_project_id, "doc missing from docsInProject set" - callback null, docLines, version, ranges, unflushedTime + callback null, docLines, version, ranges, pathname, unflushedTime getDocVersion: (doc_id, callback = (error, version) ->) -> rclient.get keys.docVersion(doc_id: doc_id), (error, version) -> diff --git a/services/document-updater/config/settings.defaults.coffee b/services/document-updater/config/settings.defaults.coffee index 9b800f2729..b1d9be6b2e 100755 --- a/services/document-updater/config/settings.defaults.coffee +++ b/services/document-updater/config/settings.defaults.coffee @@ -41,6 +41,7 @@ module.exports = projectKey: ({doc_id}) -> "ProjectId:#{doc_id}" docsInProject: ({project_id}) -> "DocsIn:#{project_id}" ranges: ({doc_id}) -> "Ranges:#{doc_id}" + pathname: ({doc_id}) -> "Pathname:#{doc_id}" projectState: ({project_id}) -> "ProjectState:#{project_id}" unflushedTime: ({doc_id}) -> "UnflushedTime:#{doc_id}" # cluster: [{ diff --git a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee index bc5f382874..62115f4bd3 100644 --- a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee @@ -31,6 +31,7 @@ describe "RedisManager", -> pendingUpdates: ({doc_id}) -> "PendingUpdates:#{doc_id}" docsInProject: ({project_id}) -> "DocsIn:#{project_id}" ranges: ({doc_id}) -> "Ranges:#{doc_id}" + pathname: ({doc_id}) -> "Pathname:#{doc_id}" projectState: ({project_id}) -> "ProjectState:#{project_id}" unflushedTime: ({doc_id}) -> "UnflushedTime:#{doc_id}" history: @@ -54,7 +55,7 @@ describe "RedisManager", -> afterEach -> tk.reset() - + @doc_id = "doc-id-123" @project_id = "project-id-123" @callback = sinon.stub() @@ -68,8 +69,9 @@ describe "RedisManager", -> @ranges = { comments: "mock", entries: "mock" } @json_ranges = JSON.stringify @ranges @unflushed_time = 12345 + @pathname = '/a/b/c.tex' @rclient.get = sinon.stub() - @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @hash, @project_id, @json_ranges, @unflushed_time]) + @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @hash, @project_id, @json_ranges, @pathname, @unflushed_time]) @rclient.sadd = sinon.stub().yields(null, 0) describe "successfully", -> @@ -80,7 +82,7 @@ describe "RedisManager", -> @rclient.get .calledWith("doclines:#{@doc_id}") .should.equal true - + it "should get the version from", -> @rclient.get .calledWith("DocVersion:#{@doc_id}") @@ -101,6 +103,11 @@ describe "RedisManager", -> .calledWith("UnflushedTime:#{@doc_id}") .should.equal true + it "should get the pathname", -> + @rclient.get + .calledWith("Pathname:#{@doc_id}") + .should.equal true + it "should check if the document is in the DocsIn set", -> @rclient.sadd .calledWith("DocsIn:#{@project_id}") @@ -108,7 +115,7 @@ describe "RedisManager", -> it 'should return the document', -> @callback - .calledWithExactly(null, @lines, @version, @ranges, @unflushed_time) + .calledWithExactly(null, @lines, @version, @ranges, @pathname, @unflushed_time) .should.equal true it 'should not log any errors', -> @@ -117,7 +124,7 @@ describe "RedisManager", -> describe "when the document is not present", -> beforeEach -> - @rclient.exec = sinon.stub().callsArgWith(0, null, [null, null, null, null, null]) + @rclient.exec = sinon.stub().callsArgWith(0, null, [null, null, null, null, null, null, null]) @rclient.sadd = sinon.stub().yields() @RedisManager.getDoc @project_id, @doc_id, @callback @@ -128,7 +135,7 @@ describe "RedisManager", -> it 'should return an empty result', -> @callback - .calledWithExactly(null, null, 0, {}) + .calledWithExactly(null, null, 0, {}, null, null) .should.equal true it 'should not log any errors', -> @@ -146,7 +153,7 @@ describe "RedisManager", -> it 'should return the document', -> @callback - .calledWithExactly(null, @lines, @version, @ranges, @unflushed_time) + .calledWithExactly(null, @lines, @version, @ranges, @pathname, @unflushed_time) .should.equal true describe "with a corrupted document", -> @@ -167,11 +174,11 @@ describe "RedisManager", -> describe "with a slow request to redis", -> beforeEach -> - @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @badHash, @project_id, @json_ranges, @unflushed_time]) + @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @badHash, @project_id, @json_ranges, @pathname, @unflushed_time]) @clock = sinon.useFakeTimers(); @rclient.exec = (cb) => @clock.tick(6000); - cb(null, [@jsonlines, @version, @another_project_id, @json_ranges, @unflushed_time]) + cb(null, [@jsonlines, @version, @another_project_id, @json_ranges, @pathname, @unflushed_time]) @RedisManager.getDoc @project_id, @doc_id, @callback @@ -186,7 +193,7 @@ describe "RedisManager", -> describe "getDoc with an invalid project id", -> beforeEach -> @another_project_id = "project-id-456" - @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @another_project_id, @json_ranges, @unflushed_time]) + @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @hash, @another_project_id, @json_ranges, @pathname, @unflushed_time]) @RedisManager.getDoc @project_id, @doc_id, @callback it 'should return an error', -> @@ -271,7 +278,7 @@ describe "RedisManager", -> @rclient.get = sinon.stub().callsArgWith(1, null, @version.toString()) @rclient.lrange = sinon.stub().callsArgWith(3, null, @jsonOps) @RedisManager.getPreviousDocOps(@doc_id, @start, @end, @callback) - + it "should return an error", -> @callback.calledWith(new Errors.OpRangeNotAvailableError("doc ops range is not loaded in redis")).should.equal true @@ -316,7 +323,7 @@ describe "RedisManager", -> @rclient.del = sinon.stub() @rclient.eval = sinon.stub() @RedisManager.getDocVersion = sinon.stub() - + @lines = ["one", "two", "three", "これは"] @ops = [{ op: [{ i: "foo", p: 4 }] },{ op: [{ i: "bar", p: 8 }] }] @version = 42 @@ -329,17 +336,17 @@ describe "RedisManager", -> beforeEach -> @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length) @RedisManager.updateDocument @doc_id, @lines, @version, @ops, @ranges, @callback - + it "should get the current doc version to check for consistency", -> @RedisManager.getDocVersion .calledWith(@doc_id) .should.equal true - + it "should set the doclines", -> @rclient.eval .calledWith(sinon.match(/redis.call/), 1, "doclines:#{@doc_id}", JSON.stringify(@lines)) .should.equal true - + it "should set the version", -> @rclient.set .calledWith("DocVersion:#{@doc_id}", @version) @@ -349,7 +356,7 @@ describe "RedisManager", -> @rclient.set .calledWith("DocHash:#{@doc_id}", @hash) .should.equal true - + it "should set the ranges", -> @rclient.set .calledWith("Ranges:#{@doc_id}", JSON.stringify(@ranges)) @@ -381,45 +388,45 @@ describe "RedisManager", -> it 'should not log any errors', -> @logger.error.calledWith() .should.equal false - + describe "with an inconsistent version", -> beforeEach -> @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length - 1) @RedisManager.updateDocument @doc_id, @lines, @version, @ops, @ranges, @callback - + it "should not call multi.exec", -> @rclient.exec.called.should.equal false - + it "should call the callback with an error", -> @callback .calledWith(new Error("Version mismatch. '#{@doc_id}' is corrupted.")) .should.equal true - + describe "with no updates", -> beforeEach -> @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version) @RedisManager.updateDocument @doc_id, @lines, @version, [], @ranges, @callback - + it "should not do an rpush", -> @rclient.rpush .called .should.equal false - + it "should still set the doclines", -> @rclient.eval .calledWith(sinon.match(/redis.call/), 1, "doclines:#{@doc_id}", JSON.stringify(@lines)) .should.equal true - + describe "with empty ranges", -> beforeEach -> @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length) @RedisManager.updateDocument @doc_id, @lines, @version, @ops, {}, @callback - + it "should not set the ranges", -> @rclient.set .calledWith("Ranges:#{@doc_id}", JSON.stringify(@ranges)) .should.equal false - + it "should delete the ranges key", -> @rclient.del .calledWith("Ranges:#{@doc_id}") @@ -448,13 +455,13 @@ describe "RedisManager", -> afterEach -> @JSON.stringify = @_stringify - + it "should log an error", -> @logger.error.called.should.equal true it "should call the callback with an error", -> @callback.calledWith(new Error("null bytes found in doc lines")).should.equal true - + describe "with ranges that are too big", -> beforeEach -> @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length) @@ -478,16 +485,17 @@ describe "RedisManager", -> @hash = crypto.createHash('sha1').update(JSON.stringify(@lines),'utf8').digest('hex') @rclient.exec = sinon.stub().callsArgWith(0, null, [@hash]) @ranges = { comments: "mock", entries: "mock" } - + @pathname = '/a/b/c.tex' + describe "with non-empty ranges", -> beforeEach (done) -> - @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, done - + @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, @pathname, done + it "should set the lines", -> @rclient.eval .calledWith(sinon.match(/redis.call/), 1, "doclines:#{@doc_id}", JSON.stringify(@lines)) .should.equal true - + it "should set the version", -> @rclient.set .calledWith("DocVersion:#{@doc_id}", @version) @@ -497,17 +505,22 @@ describe "RedisManager", -> @rclient.set .calledWith("DocHash:#{@doc_id}", @hash) .should.equal true - + it "should set the ranges", -> @rclient.set .calledWith("Ranges:#{@doc_id}", JSON.stringify(@ranges)) .should.equal true - + it "should set the project_id for the doc", -> @rclient.set .calledWith("ProjectId:#{@doc_id}", @project_id) .should.equal true - + + it "should set the pathname for the doc", -> + @rclient.set + .calledWith("Pathname:#{@doc_id}", @pathname) + .should.equal true + it "should add the doc_id to the project set", -> @rclient.sadd .calledWith("DocsIn:#{@project_id}", @doc_id) @@ -516,16 +529,16 @@ describe "RedisManager", -> it 'should not log any errors', -> @logger.error.calledWith() .should.equal false - + describe "with empty ranges", -> beforeEach (done) -> - @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, {}, done - + @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, {}, @pathname, done + it "should delete the ranges key", -> @rclient.del .calledWith("Ranges:#{@doc_id}") .should.equal true - + it "should not set the ranges", -> @rclient.set .calledWith("Ranges:#{@doc_id}", JSON.stringify(@ranges)) @@ -534,7 +547,7 @@ describe "RedisManager", -> describe "with a corrupted write", -> beforeEach (done) -> @rclient.exec = sinon.stub().callsArgWith(0, null, ["INVALID-HASH-VALUE"]) - @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, done + @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, @pathname, done it 'should log a hash error', -> @logger.error.calledWith() @@ -544,21 +557,21 @@ describe "RedisManager", -> beforeEach -> @_stringify = JSON.stringify @JSON.stringify = () -> return '["bad bytes! \u0000 <- here"]' - @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, @callback + @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, @pathname, @callback afterEach -> @JSON.stringify = @_stringify - + it "should log an error", -> @logger.error.called.should.equal true it "should call the callback with an error", -> @callback.calledWith(new Error("null bytes found in doc lines")).should.equal true - + describe "with ranges that are too big", -> beforeEach -> @RedisManager._serializeRanges = sinon.stub().yields(new Error("ranges are too large")) - @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, @callback + @RedisManager.putDocInMemory @project_id, @doc_id, @lines, @version, @ranges, @pathname, @callback it 'should log an error', -> @logger.error.called.should.equal true @@ -572,12 +585,12 @@ describe "RedisManager", -> @rclient.srem = sinon.stub() @rclient.exec.yields() @RedisManager.removeDocFromMemory @project_id, @doc_id, done - + it "should delete the lines", -> @rclient.del .calledWith("doclines:#{@doc_id}") .should.equal true - + it "should delete the version", -> @rclient.del .calledWith("DocVersion:#{@doc_id}") @@ -592,17 +605,22 @@ describe "RedisManager", -> @rclient.del .calledWith("UnflushedTime:#{@doc_id}") .should.equal true - + it "should delete the project_id for the doc", -> @rclient.del .calledWith("ProjectId:#{@doc_id}") .should.equal true - + it "should remove the doc_id from the project set", -> @rclient.srem .calledWith("DocsIn:#{@project_id}", @doc_id) .should.equal true + it "should delete the pathname for the doc", -> + @rclient.del + .calledWith("Pathname:#{@doc_id}") + .should.equal true + describe "clearProjectState", -> beforeEach (done) -> @rclient.del = sinon.stub().callsArg(1)