From ea0dd9700b6b92bbe853f48c185088cfaa5e01ee Mon Sep 17 00:00:00 2001 From: Hayden Faulds Date: Thu, 22 Feb 2018 10:01:05 +0000 Subject: [PATCH 1/3] reduce the number of times we flush project history --- .../app/coffee/DocumentManager.coffee | 9 ++++++++- .../app/coffee/HistoryManager.coffee | 6 ++---- .../app/coffee/HttpController.coffee | 8 ++++++++ .../app/coffee/ProjectManager.coffee | 6 ++++++ .../DocumentManagerTests.coffee | 18 ++++++++++++++---- .../HistoryManager/HistoryManagerTests.coffee | 17 ----------------- .../HttpController/HttpControllerTests.coffee | 12 ++++++++++++ .../flushAndDeleteProjectTests.coffee | 19 ++++++++++++++----- 8 files changed, 64 insertions(+), 31 deletions(-) diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index 8e69989d09..6574c4c4b1 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/services/document-updater/app/coffee/DocumentManager.coffee @@ -89,6 +89,13 @@ module.exports = DocumentManager = callback null else DocumentManager.flushAndDeleteDoc project_id, doc_id, (error) -> + # Flush in the background since it requires a http request. We + # want to flush project history if the previous call only failed + # to delete the doc from Redis. There is no harm in flushing + # project history if the previous call failed to flush at all. So + # do this before checking errors. + HistoryManager.flushProjectChangesAsync project_id + return callback(error) if error? callback null @@ -118,7 +125,7 @@ module.exports = DocumentManager = return callback(error) if error? # Flush in the background since it requires a http request - HistoryManager.flushChangesAsync project_id, doc_id + HistoryManager.flushDocChangesAsync project_id, doc_id RedisManager.removeDocFromMemory project_id, doc_id, (error) -> return callback(error) if error? diff --git a/services/document-updater/app/coffee/HistoryManager.coffee b/services/document-updater/app/coffee/HistoryManager.coffee index 9ec5db2aa3..6d567234da 100644 --- a/services/document-updater/app/coffee/HistoryManager.coffee +++ b/services/document-updater/app/coffee/HistoryManager.coffee @@ -4,10 +4,8 @@ logger = require "logger-sharelatex" HistoryRedisManager = require "./HistoryRedisManager" module.exports = HistoryManager = - flushChangesAsync: (project_id, doc_id) -> + flushDocChangesAsync: (project_id, doc_id) -> HistoryManager._flushDocChangesAsync project_id, doc_id - if Settings.apis?.project_history?.enabled - HistoryManager.flushProjectChangesAsync project_id _flushDocChangesAsync: (project_id, doc_id) -> if !Settings.apis?.trackchanges? @@ -23,7 +21,7 @@ module.exports = HistoryManager = logger.error { doc_id, project_id }, "track changes api returned a failure status code: #{res.statusCode}" flushProjectChangesAsync: (project_id) -> - return if !Settings.apis?.project_history? + return if !Settings.apis?.project_history?.enabled url = "#{Settings.apis.project_history.url}/project/#{project_id}/flush" logger.log { project_id, url }, "flushing doc in project history api" diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index ef9f860552..069cfc889e 100644 --- a/services/document-updater/app/coffee/HttpController.coffee +++ b/services/document-updater/app/coffee/HttpController.coffee @@ -1,4 +1,5 @@ DocumentManager = require "./DocumentManager" +HistoryManager = require "./HistoryManager" ProjectManager = require "./ProjectManager" Errors = require "./Errors" logger = require "logger-sharelatex" @@ -106,6 +107,13 @@ module.exports = HttpController = timer = new Metrics.Timer("http.deleteDoc") DocumentManager.flushAndDeleteDocWithLock project_id, doc_id, (error) -> timer.done() + # Flush in the background since it requires a http request. We + # want to flush project history if the previous call only failed + # to delete the doc from Redis. There is no harm in flushing + # project history if the previous call failed to flush at all. So + # do this before checking errors. + HistoryManager.flushProjectChangesAsync project_id + return next(error) if error? logger.log project_id: project_id, doc_id: doc_id, "deleted doc via http" res.send 204 # No Content diff --git a/services/document-updater/app/coffee/ProjectManager.coffee b/services/document-updater/app/coffee/ProjectManager.coffee index af2e27c8e5..a82d88b4a6 100644 --- a/services/document-updater/app/coffee/ProjectManager.coffee +++ b/services/document-updater/app/coffee/ProjectManager.coffee @@ -59,6 +59,12 @@ module.exports = ProjectManager = logger.log project_id: project_id, doc_ids: doc_ids, "deleting docs" async.series jobs, () -> + # Flush in the background since it requires a htpt request. If we + # flushed and deleted only some docs successfully then we should still + # flush project history. If no docs succeeded then there is still no + # harm flushing project history. So do this before checking errors. + HistoryManager.flushProjectChangesAsync project_id + if errors.length > 0 callback new Error("Errors deleting docs. See log for details") else diff --git a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee index 702617f7ae..c390138cf9 100644 --- a/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/DocumentManager/DocumentManagerTests.coffee @@ -11,7 +11,9 @@ describe "DocumentManager", -> @DocumentManager = SandboxedModule.require modulePath, requires: "./RedisManager": @RedisManager = {} "./PersistenceManager": @PersistenceManager = {} - "./HistoryManager": @HistoryManager = {} + "./HistoryManager": @HistoryManager = + flushDocChangesAsync: sinon.stub() + flushProjectChangesAsync: sinon.stub() "logger-sharelatex": @logger = {log: sinon.stub()} "./DocOpsManager": @DocOpsManager = {} "./Metrics": @Metrics = @@ -36,7 +38,6 @@ describe "DocumentManager", -> beforeEach -> @RedisManager.removeDocFromMemory = sinon.stub().callsArg(2) @DocumentManager.flushDocIfLoaded = sinon.stub().callsArgWith(2) - @HistoryManager.flushChangesAsync = sinon.stub() @DocumentManager.flushAndDeleteDoc @project_id, @doc_id, @callback it "should flush the doc", -> @@ -56,8 +57,8 @@ describe "DocumentManager", -> @Metrics.Timer::done.called.should.equal true it "should flush to the history api", -> - @HistoryManager.flushChangesAsync - .calledWith(@project_id, @doc_id) + @HistoryManager.flushDocChangesAsync + .calledWithExactly(@project_id, @doc_id) .should.equal true describe "flushDocIfLoaded", -> @@ -243,6 +244,10 @@ describe "DocumentManager", -> .calledWith(@project_id, @doc_id) .should.equal true + it "should not flush the project history", -> + @HistoryManager.flushProjectChangesAsync + .called.should.equal false + it "should call the callback", -> @callback.calledWith(null).should.equal true @@ -259,6 +264,11 @@ describe "DocumentManager", -> .calledWith(@project_id, @doc_id) .should.equal true + it "should not flush the project history", -> + @HistoryManager.flushProjectChangesAsync + .calledWithExactly(@project_id) + .should.equal true + describe "without new lines", -> beforeEach -> @DocumentManager.setDoc @project_id, @doc_id, null, @source, @user_id, false, @callback diff --git a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee b/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee index 75327a7ae9..161b7afd44 100644 --- a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee @@ -21,23 +21,6 @@ describe "HistoryManager", -> @doc_id = "mock-doc-id" @callback = sinon.stub() - describe "flushChangesAsync", -> - beforeEach -> - @HistoryManager._flushDocChangesAsync = sinon.stub() - @HistoryManager.flushProjectChangesAsync = sinon.stub() - - @HistoryManager.flushChangesAsync(@project_id, @doc_id) - - it "flushes doc changes", -> - @HistoryManager._flushDocChangesAsync - .calledWith(@project_id, @doc_id) - .should.equal true - - it "flushes project changes", -> - @HistoryManager.flushProjectChangesAsync - .calledWith(@project_id) - .should.equal true - describe "_flushDocChangesAsync", -> beforeEach -> @request.post = sinon.stub().callsArgWith(1, null, statusCode: 204) diff --git a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee index d52956635d..99496332fc 100644 --- a/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee +++ b/services/document-updater/test/unit/coffee/HttpController/HttpControllerTests.coffee @@ -9,6 +9,8 @@ describe "HttpController", -> beforeEach -> @HttpController = SandboxedModule.require modulePath, requires: "./DocumentManager": @DocumentManager = {} + "./HistoryManager": @HistoryManager = + flushProjectChangesAsync: sinon.stub() "./ProjectManager": @ProjectManager = {} "logger-sharelatex" : @logger = { log: sinon.stub() } "./Metrics": @Metrics = {} @@ -275,6 +277,11 @@ describe "HttpController", -> .calledWith(@project_id, @doc_id) .should.equal true + it "should flush project history", -> + @HistoryManager.flushProjectChangesAsync + .calledWithExactly(@project_id) + .should.equal true + it "should return a successful No Content response", -> @res.send .calledWith(204) @@ -293,6 +300,11 @@ describe "HttpController", -> @DocumentManager.flushAndDeleteDocWithLock = sinon.stub().callsArgWith(2, new Error("oops")) @HttpController.flushAndDeleteDoc(@req, @res, @next) + it "should flush project history", -> + @HistoryManager.flushProjectChangesAsync + .calledWithExactly(@project_id) + .should.equal true + it "should call next with the error", -> @next .calledWith(new Error("oops")) diff --git a/services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee b/services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee index 74161ca4a2..50a2679953 100644 --- a/services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee +++ b/services/document-updater/test/unit/coffee/ProjectManager/flushAndDeleteProjectTests.coffee @@ -10,7 +10,8 @@ describe "ProjectManager - flushAndDeleteProject", -> "./RedisManager": @RedisManager = {} "./DocumentManager": @DocumentManager = {} "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() } - "./HistoryManager": @HistoryManager = {} + "./HistoryManager": @HistoryManager = + flushProjectChangesAsync: sinon.stub() "./Metrics": @Metrics = Timer: class Timer done: sinon.stub() @@ -30,13 +31,18 @@ describe "ProjectManager - flushAndDeleteProject", -> @RedisManager.getDocIdsInProject .calledWith(@project_id) .should.equal true - + it "should delete each doc in the project", -> for doc_id in @doc_ids @DocumentManager.flushAndDeleteDocWithLock .calledWith(@project_id, doc_id) .should.equal true + it "should flush project history", -> + @HistoryManager.flushProjectChangesAsync + .calledWithExactly(@project_id) + .should.equal true + it "should call the callback without error", -> @callback.calledWith(null).should.equal true @@ -55,13 +61,18 @@ describe "ProjectManager - flushAndDeleteProject", -> @ProjectManager.flushAndDeleteProjectWithLocks @project_id, (error) => @callback(error) done() - + it "should still flush each doc in the project", -> for doc_id in @doc_ids @DocumentManager.flushAndDeleteDocWithLock .calledWith(@project_id, doc_id) .should.equal true + it "should still flush project history", -> + @HistoryManager.flushProjectChangesAsync + .calledWithExactly(@project_id) + .should.equal true + it "should record the error", -> @logger.error .calledWith(err: @error, project_id: @project_id, doc_id: "doc-id-1", "error deleting doc") @@ -72,5 +83,3 @@ describe "ProjectManager - flushAndDeleteProject", -> it "should time the execution", -> @Metrics.Timer::done.called.should.equal true - - From 0f87ae1f742a7f6ababa59e2d89205edf2e79bf2 Mon Sep 17 00:00:00 2001 From: Hayden Faulds Date: Thu, 22 Feb 2018 10:16:29 +0000 Subject: [PATCH 2/3] simplify comments --- .../document-updater/app/coffee/DocumentManager.coffee | 7 ++----- services/document-updater/app/coffee/HttpController.coffee | 7 ++----- services/document-updater/app/coffee/ProjectManager.coffee | 6 ++---- 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/services/document-updater/app/coffee/DocumentManager.coffee b/services/document-updater/app/coffee/DocumentManager.coffee index 6574c4c4b1..3d3f690b5c 100644 --- a/services/document-updater/app/coffee/DocumentManager.coffee +++ b/services/document-updater/app/coffee/DocumentManager.coffee @@ -89,11 +89,8 @@ module.exports = DocumentManager = callback null else DocumentManager.flushAndDeleteDoc project_id, doc_id, (error) -> - # Flush in the background since it requires a http request. We - # want to flush project history if the previous call only failed - # to delete the doc from Redis. There is no harm in flushing - # project history if the previous call failed to flush at all. So - # do this before checking errors. + # There is no harm in flushing project history if the previous + # call failed and sometimes it is required HistoryManager.flushProjectChangesAsync project_id return callback(error) if error? diff --git a/services/document-updater/app/coffee/HttpController.coffee b/services/document-updater/app/coffee/HttpController.coffee index 069cfc889e..650ee07ae2 100644 --- a/services/document-updater/app/coffee/HttpController.coffee +++ b/services/document-updater/app/coffee/HttpController.coffee @@ -107,11 +107,8 @@ module.exports = HttpController = timer = new Metrics.Timer("http.deleteDoc") DocumentManager.flushAndDeleteDocWithLock project_id, doc_id, (error) -> timer.done() - # Flush in the background since it requires a http request. We - # want to flush project history if the previous call only failed - # to delete the doc from Redis. There is no harm in flushing - # project history if the previous call failed to flush at all. So - # do this before checking errors. + # There is no harm in flushing project history if the previous call + # failed and sometimes it is required HistoryManager.flushProjectChangesAsync project_id return next(error) if error? diff --git a/services/document-updater/app/coffee/ProjectManager.coffee b/services/document-updater/app/coffee/ProjectManager.coffee index a82d88b4a6..64293e6985 100644 --- a/services/document-updater/app/coffee/ProjectManager.coffee +++ b/services/document-updater/app/coffee/ProjectManager.coffee @@ -59,10 +59,8 @@ module.exports = ProjectManager = logger.log project_id: project_id, doc_ids: doc_ids, "deleting docs" async.series jobs, () -> - # Flush in the background since it requires a htpt request. If we - # flushed and deleted only some docs successfully then we should still - # flush project history. If no docs succeeded then there is still no - # harm flushing project history. So do this before checking errors. + # There is no harm in flushing project history if the previous call + # failed and sometimes it is required HistoryManager.flushProjectChangesAsync project_id if errors.length > 0 From 5a11332aa3609f031b3f51d72f7ac7eead747fad Mon Sep 17 00:00:00 2001 From: Hayden Faulds Date: Thu, 22 Feb 2018 10:16:41 +0000 Subject: [PATCH 3/3] remove unecessary method wrapping --- .../app/coffee/HistoryManager.coffee | 5 +---- .../HistoryManager/HistoryManagerTests.coffee | 14 +++++++------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/services/document-updater/app/coffee/HistoryManager.coffee b/services/document-updater/app/coffee/HistoryManager.coffee index 6d567234da..3ccc42e97f 100644 --- a/services/document-updater/app/coffee/HistoryManager.coffee +++ b/services/document-updater/app/coffee/HistoryManager.coffee @@ -5,9 +5,6 @@ HistoryRedisManager = require "./HistoryRedisManager" module.exports = HistoryManager = flushDocChangesAsync: (project_id, doc_id) -> - HistoryManager._flushDocChangesAsync project_id, doc_id - - _flushDocChangesAsync: (project_id, doc_id) -> if !Settings.apis?.trackchanges? logger.warn { doc_id }, "track changes API is not configured, so not flushing" return @@ -51,7 +48,7 @@ module.exports = HistoryManager = # Do this in the background since it uses HTTP and so may be too # slow to wait for when processing a doc update. logger.log { doc_ops_length, doc_id, project_id }, "flushing track changes api" - HistoryManager._flushDocChangesAsync project_id, doc_id + HistoryManager.flushDocChangesAsync project_id, doc_id callback() shouldFlushHistoryOps: (length, ops_length, threshold) -> diff --git a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee b/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee index 161b7afd44..6e5010d89c 100644 --- a/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/HistoryManager/HistoryManagerTests.coffee @@ -21,11 +21,11 @@ describe "HistoryManager", -> @doc_id = "mock-doc-id" @callback = sinon.stub() - describe "_flushDocChangesAsync", -> + describe "flushDocChangesAsync", -> beforeEach -> @request.post = sinon.stub().callsArgWith(1, null, statusCode: 204) - @HistoryManager._flushDocChangesAsync @project_id, @doc_id + @HistoryManager.flushDocChangesAsync @project_id, @doc_id it "should send a request to the track changes api", -> @request.post @@ -51,7 +51,7 @@ describe "HistoryManager", -> @HistoryManager.flushProjectChangesAsync = sinon.stub() @HistoryRedisManager.recordDocHasHistoryOps = sinon.stub().callsArg(3) - @HistoryManager._flushDocChangesAsync = sinon.stub() + @HistoryManager.flushDocChangesAsync = sinon.stub() describe "with no ops", -> beforeEach -> @@ -66,7 +66,7 @@ describe "HistoryManager", -> @HistoryRedisManager.recordDocHasHistoryOps.called.should.equal false it "should not flush doc changes", -> - @HistoryManager._flushDocChangesAsync.called.should.equal false + @HistoryManager.flushDocChangesAsync.called.should.equal false it "should call the callback", -> @callback.called.should.equal true @@ -91,7 +91,7 @@ describe "HistoryManager", -> .calledWith(@project_id, @doc_id, @ops) it "should not flush doc changes", -> - @HistoryManager._flushDocChangesAsync.called.should.equal false + @HistoryManager.flushDocChangesAsync.called.should.equal false it "should call the callback", -> @callback.called.should.equal true @@ -114,7 +114,7 @@ describe "HistoryManager", -> .calledWith(@project_id, @doc_id, @ops) it "should flush doc changes", -> - @HistoryManager._flushDocChangesAsync + @HistoryManager.flushDocChangesAsync .calledWith(@project_id, @doc_id) .should.equal true @@ -132,7 +132,7 @@ describe "HistoryManager", -> ) it "should not flush doc changes", -> - @HistoryManager._flushDocChangesAsync.called.should.equal false + @HistoryManager.flushDocChangesAsync.called.should.equal false it "should call the callback with the error", -> @callback.calledWith(@error).should.equal true