From 178440395f2131ffedb9ba26b6aa399cc366c25c Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Sun, 28 Mar 2021 13:18:21 +0200 Subject: [PATCH] [perf] switch write sequence for doc contents and doc tracking Doc contents are added only after the tracking has been setup. All read paths on the tracking have been checked to gracefully handle the case of existing doc_id but missing doc contents. - getDoc: -1 operation REF: 0a2b47c660c60b95e360d8f3b3e30b862ceb6d79 --- .../document-updater/app/js/RedisManager.js | 90 ++++++------------- .../unit/js/RedisManager/RedisManagerTests.js | 78 ---------------- 2 files changed, 27 insertions(+), 141 deletions(-) diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index 35f62c8222..4c0d144529 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -84,28 +84,23 @@ module.exports = RedisManager = { logger.error({ err: error, doc_id, project_id }, error.message) return callback(error) } - const multi = rclient.multi() - multi.set(keys.docLines({ doc_id }), docLines) - multi.set(keys.projectKey({ doc_id }), project_id) - multi.set(keys.docVersion({ doc_id }), version) - multi.set(keys.docHash({ doc_id }), docHash) - if (ranges != null) { - multi.set(keys.ranges({ doc_id }), ranges) - } else { - multi.del(keys.ranges({ doc_id })) - } - multi.set(keys.pathname({ doc_id }), pathname) - multi.set(keys.projectHistoryId({ doc_id }), projectHistoryId) - return multi.exec(function (error, result) { - if (error != null) { - return callback(error) + // update docsInProject set before writing doc contents + rclient.sadd(keys.docsInProject({ project_id }), doc_id, (error) => { + if (error) return callback(error) + + const multi = rclient.multi() + multi.set(keys.docLines({ doc_id }), docLines) + multi.set(keys.projectKey({ doc_id }), project_id) + multi.set(keys.docVersion({ doc_id }), version) + multi.set(keys.docHash({ doc_id }), docHash) + if (ranges != null) { + multi.set(keys.ranges({ doc_id }), ranges) + } else { + multi.del(keys.ranges({ doc_id })) } - // update docsInProject set - return rclient.sadd( - keys.docsInProject({ project_id }), - doc_id, - callback - ) + multi.set(keys.pathname({ doc_id }), pathname) + multi.set(keys.projectHistoryId({ doc_id }), projectHistoryId) + multi.exec(callback) }) }) }, @@ -269,48 +264,17 @@ module.exports = RedisManager = { projectHistoryId = parseInt(projectHistoryId) } - // doc is not in redis, bail out - if (docLines == null) { - return callback( - null, - docLines, - version, - ranges, - pathname, - projectHistoryId, - unflushedTime, - lastUpdatedAt, - lastUpdatedBy - ) - } - - // doc should be in project set, check if missing (workaround for missing docs from putDoc) - return rclient.sadd(keys.docsInProject({ project_id }), doc_id, function ( - error, - result - ) { - if (error != null) { - return callback(error) - } - if (result !== 0) { - // doc should already be in set - logger.error( - { project_id, doc_id, doc_project_id }, - 'doc missing from docsInProject set' - ) - } - return callback( - null, - docLines, - version, - ranges, - pathname, - projectHistoryId, - unflushedTime, - lastUpdatedAt, - lastUpdatedBy - ) - }) + callback( + null, + docLines, + version, + ranges, + pathname, + projectHistoryId, + unflushedTime, + lastUpdatedAt, + lastUpdatedBy + ) }) }, diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index 1937ddfb86..d8f21844cd 100644 --- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js +++ b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js @@ -182,12 +182,6 @@ describe('RedisManager', function () { .should.equal(true) }) - it('should check if the document is in the DocsIn set', function () { - return this.rclient.sadd - .calledWith(`DocsIn:${this.project_id}`) - .should.equal(true) - }) - it('should return the document', function () { return this.callback .calledWithExactly( @@ -209,78 +203,6 @@ describe('RedisManager', function () { }) }) - describe('when the document is not present', function () { - beforeEach(function () { - this.rclient.mget = sinon - .stub() - .yields(null, [ - null, - null, - null, - null, - null, - null, - null, - null, - null, - null - ]) - this.rclient.sadd = sinon.stub().yields() - return this.RedisManager.getDoc( - this.project_id, - this.doc_id, - this.callback - ) - }) - - it('should not check if the document is in the DocsIn set', function () { - return this.rclient.sadd - .calledWith(`DocsIn:${this.project_id}`) - .should.equal(false) - }) - - it('should return an empty result', function () { - return this.callback - .calledWithExactly(null, null, 0, {}, null, null, null, null, null) - .should.equal(true) - }) - - return it('should not log any errors', function () { - return this.logger.error.calledWith().should.equal(false) - }) - }) - - describe('when the document is missing from the DocsIn set', function () { - beforeEach(function () { - this.rclient.sadd = sinon.stub().yields(null, 1) - return this.RedisManager.getDoc( - this.project_id, - this.doc_id, - this.callback - ) - }) - - it('should log an error', function () { - return this.logger.error.calledWith().should.equal(true) - }) - - return it('should return the document', function () { - return this.callback - .calledWithExactly( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.unflushed_time, - this.lastUpdatedAt, - this.lastUpdatedBy - ) - .should.equal(true) - }) - }) - describe('with a corrupted document', function () { beforeEach(function () { this.badHash = 'INVALID-HASH-VALUE'