From 68e12f4d2d8b9377f927e97652e8e666388b48f8 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 25 Nov 2019 10:51:10 +0000 Subject: [PATCH 1/3] add metrics for queue operations --- services/document-updater/app/coffee/RedisManager.coffee | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/services/document-updater/app/coffee/RedisManager.coffee b/services/document-updater/app/coffee/RedisManager.coffee index 7729486986..01e0289bee 100644 --- a/services/document-updater/app/coffee/RedisManager.coffee +++ b/services/document-updater/app/coffee/RedisManager.coffee @@ -259,9 +259,11 @@ module.exports = RedisManager = # expire must come after rpush since before it will be a no-op if the list is empty multi.expire keys.docOps(doc_id: doc_id), RedisManager.DOC_OPS_TTL # index 6 if projectHistoryType is "project-history" - logger.debug {doc_id}, "skipping push of uncompressed ops for project using project-history" + metrics.inc 'history-queue', 1, {status: 'skip-track-changes'} + logger.log {doc_id}, "skipping push of uncompressed ops for project using project-history" else # project is using old track-changes history service + metrics.inc 'history-queue', 1, {status: 'track-changes'} multi.rpush historyKeys.uncompressedHistoryOps(doc_id: doc_id), jsonOps... # index 7 # Set the unflushed timestamp to the current time if the doc # hasn't been modified before (the content in mongo has been @@ -282,6 +284,7 @@ module.exports = RedisManager = docUpdateCount = result[7] # length of uncompressedHistoryOps queue (index 7) if jsonOps.length > 0 && Settings.apis?.project_history?.enabled + metrics.inc 'history-queue', 1, {status: 'project-history'} ProjectHistoryRedisManager.queueOps project_id, jsonOps..., (error, projectUpdateCount) -> callback null, docUpdateCount, projectUpdateCount else From 4f6583bbf22877dafc37d9d71acaded07604f0d8 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 25 Nov 2019 13:28:36 +0000 Subject: [PATCH 2/3] fix getDocVersion and add tests --- .../app/coffee/RedisManager.coffee | 3 +- .../coffee/ApplyingUpdatesToADocTests.coffee | 33 ++++++++++++++ .../RedisManager/RedisManagerTests.coffee | 44 ++++++++++++++++++- 3 files changed, 78 insertions(+), 2 deletions(-) diff --git a/services/document-updater/app/coffee/RedisManager.coffee b/services/document-updater/app/coffee/RedisManager.coffee index 01e0289bee..9e2edbd99d 100644 --- a/services/document-updater/app/coffee/RedisManager.coffee +++ b/services/document-updater/app/coffee/RedisManager.coffee @@ -156,8 +156,9 @@ module.exports = RedisManager = callback null, docLines, version, ranges, pathname, projectHistoryId, unflushedTime, lastUpdatedAt, lastUpdatedBy getDocVersion: (doc_id, callback = (error, version, projectHistoryType) ->) -> - rclient.mget keys.docVersion(doc_id: doc_id), keys.projectHistoryType(doc_id:doc_id), (error, version, projectHistoryType) -> + rclient.mget keys.docVersion(doc_id: doc_id), keys.projectHistoryType(doc_id:doc_id), (error, result) -> return callback(error) if error? + [version, projectHistoryType] = result || [] version = parseInt(version, 10) callback null, version, projectHistoryType diff --git a/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee b/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee index 51b9cf08a9..0b28dea7a7 100644 --- a/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee +++ b/services/document-updater/test/acceptance/coffee/ApplyingUpdatesToADocTests.coffee @@ -135,6 +135,39 @@ describe "Applying updates to a doc", -> done() return null + describe "when the document is loaded and is using project-history only", -> + before (done) -> + [@project_id, @doc_id] = [DocUpdaterClient.randomId(), DocUpdaterClient.randomId()] + + MockWebApi.insertDoc @project_id, @doc_id, {lines: @lines, version: @version, projectHistoryType: 'project-history'} + DocUpdaterClient.preloadDoc @project_id, @doc_id, (error) => + throw error if error? + sinon.spy MockWebApi, "getDocument" + DocUpdaterClient.sendUpdate @project_id, @doc_id, @update, (error) -> + throw error if error? + setTimeout done, 200 + return null + + after -> + MockWebApi.getDocument.restore() + + it "should update the doc", (done) -> + DocUpdaterClient.getDoc @project_id, @doc_id, (error, res, doc) => + doc.lines.should.deep.equal @result + done() + return null + + it "should not push any applied updates to the track changes api", (done) -> + rclient_history.lrange HistoryKeys.uncompressedHistoryOps({@doc_id}), 0, -1, (error, updates) => + updates.length.should.equal 0 + done() + return null + + it "should push the applied updates to the project history changes api", (done) -> + rclient_history.lrange ProjectHistoryKeys.projectHistoryOps({@project_id}), 0, -1, (error, updates) => + JSON.parse(updates[0]).op.should.deep.equal @update.op + done() + return null describe "when the document has been deleted", -> describe "when the ops come in a single linear order", -> diff --git a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee index 508a9ba0f7..99035a32b3 100644 --- a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee @@ -361,11 +361,12 @@ describe "RedisManager", -> describe "with a consistent version", -> beforeEach -> - @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length) + describe "with project history enabled", -> beforeEach -> @settings.apis.project_history.enabled = true + @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length) @RedisManager.updateDocument @project_id, @doc_id, @lines, @version, @ops, @ranges, @updateMeta, @callback it "should get the current doc version to check for consistency", -> @@ -446,6 +447,7 @@ describe "RedisManager", -> beforeEach -> @rclient.rpush = sinon.stub() @settings.apis.project_history.enabled = false + @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length) @RedisManager.updateDocument @project_id, @doc_id, @lines, @version, @ops, @ranges, @updateMeta, @callback it "should not push the updates into the project history ops list", -> @@ -456,6 +458,26 @@ describe "RedisManager", -> .calledWith(null, @doc_update_list_length) .should.equal true + describe "with a doc using project history only", -> + beforeEach -> + @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length, 'project-history') + @RedisManager.updateDocument @project_id, @doc_id, @lines, @version, @ops, @ranges, @updateMeta, @callback + + it "should not push the updates to the track-changes ops list", -> + @multi.rpush + .calledWith("UncompressedHistoryOps:#{@doc_id}") + .should.equal false + + it "should push the updates into the project history ops list", -> + @ProjectHistoryRedisManager.queueOps + .calledWith(@project_id, JSON.stringify(@ops[0])) + .should.equal true + + it "should call the callback with the project update count only", -> + @callback + .calledWith(null, undefined, @project_update_list_length) + .should.equal true + describe "with an inconsistent version", -> beforeEach -> @RedisManager.getDocVersion.withArgs(@doc_id).yields(null, @version - @ops.length - 1) @@ -754,3 +776,23 @@ describe "RedisManager", -> @ProjectHistoryRedisManager.queueRenameEntity .calledWithExactly(@project_id, @projectHistoryId, 'doc', @doc_id, @userId, @update, @callback) .should.equal true + + describe "getDocVersion", -> + beforeEach -> + @version = 12345 + + describe "when the document does not have a project history type set", -> + beforeEach -> + @rclient.mget = sinon.stub().withArgs("DocVersion:#{@doc_id}", "ProjectHistoryType:#{@doc_id}").callsArgWith(2, null, ["#{@version}"]) + @RedisManager.getDocVersion @doc_id, @callback + + it "should return the document version and an undefined history type", -> + @callback.calledWithExactly(null, @version, undefined).should.equal true + + describe "when the document has a project history type set", -> + beforeEach -> + @rclient.mget = sinon.stub().withArgs("DocVersion:#{@doc_id}", "ProjectHistoryType:#{@doc_id}").callsArgWith(2, null, ["#{@version}", 'project-history']) + @RedisManager.getDocVersion @doc_id, @callback + + it "should return the document version and history type", -> + @callback.calledWithExactly(null, @version, 'project-history').should.equal true From ad19fee66770773027a14c1d6eeeb436cca07cc3 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 25 Nov 2019 13:36:25 +0000 Subject: [PATCH 3/3] add setting so that double flush is the default can be disabled to stop flushing to track-changes --- .../app/coffee/HistoryManager.coffee | 2 +- .../config/settings.defaults.coffee | 2 ++ .../HistoryManager/HistoryManagerTests.coffee | 14 +++++++++++++- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/services/document-updater/app/coffee/HistoryManager.coffee b/services/document-updater/app/coffee/HistoryManager.coffee index 0e13627445..183ac268f3 100644 --- a/services/document-updater/app/coffee/HistoryManager.coffee +++ b/services/document-updater/app/coffee/HistoryManager.coffee @@ -16,7 +16,7 @@ module.exports = HistoryManager = if err? logger.warn {err, doc_id}, "error getting history type" # if there's an error continue and flush to track-changes for safety - if projectHistoryType is "project-history" + if Settings.disableDoubleFlush and projectHistoryType is "project-history" logger.debug {doc_id, projectHistoryType}, "skipping track-changes flush" else metrics.inc 'history-flush', 1, { status: 'track-changes'} diff --git a/services/document-updater/config/settings.defaults.coffee b/services/document-updater/config/settings.defaults.coffee index e8804dfe3e..9eebe86005 100755 --- a/services/document-updater/config/settings.defaults.coffee +++ b/services/document-updater/config/settings.defaults.coffee @@ -93,3 +93,5 @@ module.exports = continuousBackgroundFlush: process.env['CONTINUOUS_BACKGROUND_FLUSH'] or false smoothingOffset: process.env['SMOOTHING_OFFSET'] or 1000 # milliseconds + + disableDoubleFlush: process.env['DISABLE_DOUBLE_FLUSH'] or false # don't flush track-changes for projects using project-history diff --git a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee b/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee index 4a738bd455..6cb6b1d8da 100644 --- a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee @@ -39,16 +39,28 @@ describe "HistoryManager", -> .calledWith("#{@Settings.apis.trackchanges.url}/project/#{@project_id}/doc/#{@doc_id}/flush") .should.equal true - describe "when the project uses project history", -> + describe "when the project uses project history and double flush is not disabled", -> beforeEach -> @RedisManager.getHistoryType = sinon.stub().yields(null, 'project-history') @HistoryManager.flushDocChangesAsync @project_id, @doc_id + it "should send a request to the track changes api", -> + @request.post + .called + .should.equal true + + describe "when the project uses project history and double flush is disabled", -> + beforeEach -> + @Settings.disableDoubleFlush = true + @RedisManager.getHistoryType = sinon.stub().yields(null, 'project-history') + @HistoryManager.flushDocChangesAsync @project_id, @doc_id + it "should not send a request to the track changes api", -> @request.post .called .should.equal false + describe "flushProjectChangesAsync", -> beforeEach -> @request.post = sinon.stub().callsArgWith(1, null, statusCode: 204)