From 22257cf037aab7820a020bd77c534a842bd090d2 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Wed, 29 May 2024 11:50:34 -0400 Subject: [PATCH] Merge pull request #18583 from overleaf/em-multi-hash-key Ensure that MULTI commands always go to a single node GitOrigin-RevId: 223d29986766577df89c82983126dabca5d16eed --- .../app/js/ProjectHistoryRedisManager.js | 6 + .../app/js/RealTimeRedisManager.js | 5 + .../document-updater/app/js/RedisManager.js | 135 +++++++++++------ .../test/acceptance/js/RangesTests.js | 139 +++++++++--------- .../acceptance/js/helpers/DocUpdaterClient.js | 4 - .../unit/js/RedisManager/RedisManagerTests.js | 9 +- 6 files changed, 172 insertions(+), 126 deletions(-) diff --git a/services/document-updater/app/js/ProjectHistoryRedisManager.js b/services/document-updater/app/js/ProjectHistoryRedisManager.js index 2cfaf33b9d..d0802182ab 100644 --- a/services/document-updater/app/js/ProjectHistoryRedisManager.js +++ b/services/document-updater/app/js/ProjectHistoryRedisManager.js @@ -23,6 +23,12 @@ const ProjectHistoryRedisManager = { for (const op of ops) { metrics.summary('redis.projectHistoryOps', op.length, { status: 'push' }) } + + // Make sure that this MULTI operation only operates on project + // specific keys, i.e. keys that have the project id in curly braces. + // The curly braces identify a hash key for Redis and ensures that + // the MULTI's operations are all done on the same node in a + // cluster environment. const multi = rclient.multi() // Push the ops onto the project history queue multi.rpush( diff --git a/services/document-updater/app/js/RealTimeRedisManager.js b/services/document-updater/app/js/RealTimeRedisManager.js index 32f15a546d..72713e3ffa 100644 --- a/services/document-updater/app/js/RealTimeRedisManager.js +++ b/services/document-updater/app/js/RealTimeRedisManager.js @@ -32,6 +32,11 @@ const MAX_OPS_PER_ITERATION = 8 // process a limited number of ops for safety const RealTimeRedisManager = { getPendingUpdatesForDoc(docId, callback) { + // Make sure that this MULTI operation only operates on doc + // specific keys, i.e. keys that have the doc id in curly braces. + // The curly braces identify a hash key for Redis and ensures that + // the MULTI's operations are all done on the same node in a + // cluster environment. const multi = rclient.multi() multi.lrange( Keys.pendingUpdates({ doc_id: docId }), diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index 2c7586eb12..c504a213cf 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -77,57 +77,66 @@ const RedisManager = { logger.error({ err: error, docId, projectId }, error.message) return callback(error) } - // update docsInProject set before writing doc contents - rclient.sadd( - keys.docsInProject({ project_id: projectId }), - docId, - error => { - if (error) return callback(error) + setHistoryRangesSupportFlag(docId, historyRangesSupport, err => { + if (err) { + return callback(err) + } + // update docsInProject set before writing doc contents + rclient.sadd( + keys.docsInProject({ project_id: projectId }), + docId, + error => { + if (error) return callback(error) - if (!pathname) { - metrics.inc('pathname', 1, { - path: 'RedisManager.setDoc', - status: pathname === '' ? 'zero-length' : 'undefined', + if (!pathname) { + metrics.inc('pathname', 1, { + path: 'RedisManager.setDoc', + status: pathname === '' ? 'zero-length' : 'undefined', + }) + } + + // Make sure that this MULTI operation only operates on doc + // specific keys, i.e. keys that have the doc id in curly braces. + // The curly braces identify a hash key for Redis and ensures that + // the MULTI's operations are all done on the same node in a + // cluster environment. + const multi = rclient.multi() + multi.mset({ + [keys.docLines({ doc_id: docId })]: docLines, + [keys.projectKey({ doc_id: docId })]: projectId, + [keys.docVersion({ doc_id: docId })]: version, + [keys.docHash({ doc_id: docId })]: docHash, + [keys.ranges({ doc_id: docId })]: ranges, + [keys.pathname({ doc_id: docId })]: pathname, + [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId, + }) + if (historyRangesSupport) { + multi.del(keys.resolvedCommentIds({ doc_id: docId })) + if (resolvedCommentIds.length > 0) { + multi.sadd( + keys.resolvedCommentIds({ doc_id: docId }), + ...resolvedCommentIds + ) + } + } + multi.exec(err => { + if (err) { + callback( + OError.tag(err, 'failed to write doc to Redis in MULTI', { + previousErrors: err.previousErrors.map(e => ({ + name: e.name, + message: e.message, + command: e.command, + })), + }) + ) + } else { + callback() + } }) } - - const multi = rclient.multi() - multi.mset({ - [keys.docLines({ doc_id: docId })]: docLines, - [keys.projectKey({ doc_id: docId })]: projectId, - [keys.docVersion({ doc_id: docId })]: version, - [keys.docHash({ doc_id: docId })]: docHash, - [keys.ranges({ doc_id: docId })]: ranges, - [keys.pathname({ doc_id: docId })]: pathname, - [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId, - }) - if (historyRangesSupport) { - multi.sadd(keys.historyRangesSupport(), docId) - multi.del(keys.resolvedCommentIds({ doc_id: docId })) - if (resolvedCommentIds.length > 0) { - multi.sadd( - keys.resolvedCommentIds({ doc_id: docId }), - ...resolvedCommentIds - ) - } - } - multi.exec(err => { - if (err) { - callback( - OError.tag(err, 'failed to write doc to Redis in MULTI', { - previousErrors: err.previousErrors.map(e => ({ - name: e.name, - message: e.message, - command: e.command, - })), - }) - ) - } else { - callback() - } - }) - } - ) + ) + }) }) }, @@ -143,6 +152,11 @@ const RedisManager = { } } + // Make sure that this MULTI operation only operates on doc + // specific keys, i.e. keys that have the doc id in curly braces. + // The curly braces identify a hash key for Redis and ensures that + // the MULTI's operations are all done on the same node in a + // cluster environment. let multi = rclient.multi() multi.strlen(keys.docLines({ doc_id: docId })) multi.del( @@ -167,6 +181,12 @@ const RedisManager = { // record bytes freed in redis metrics.summary('redis.docLines', length, { status: 'del' }) } + + // Make sure that this MULTI operation only operates on project + // specific keys, i.e. keys that have the project id in curly braces. + // The curly braces identify a hash key for Redis and ensures that + // the MULTI's operations are all done on the same node in a + // cluster environment. multi = rclient.multi() multi.srem(keys.docsInProject({ project_id: projectId }), docId) multi.del(keys.projectState({ project_id: projectId })) @@ -180,6 +200,11 @@ const RedisManager = { }, checkOrSetProjectState(projectId, newState, callback) { + // Make sure that this MULTI operation only operates on project + // specific keys, i.e. keys that have the project id in curly braces. + // The curly braces identify a hash key for Redis and ensures that + // the MULTI's operations are all done on the same node in a + // cluster environment. const multi = rclient.multi() multi.getset(keys.projectState({ project_id: projectId }), newState) multi.expire(keys.projectState({ project_id: projectId }), 30 * minutes) @@ -488,6 +513,12 @@ const RedisManager = { logger.error({ err: error, docId, ranges }, error.message) return callback(error) } + + // Make sure that this MULTI operation only operates on doc + // specific keys, i.e. keys that have the doc id in curly braces. + // The curly braces identify a hash key for Redis and ensures that + // the MULTI's operations are all done on the same node in a + // cluster environment. const multi = rclient.multi() multi.mset({ [keys.docLines({ doc_id: docId })]: newDocLines, @@ -670,6 +701,14 @@ const RedisManager = { }, } +function setHistoryRangesSupportFlag(docId, historyRangesSupport, callback) { + if (historyRangesSupport) { + rclient.sadd(keys.historyRangesSupport(), docId, callback) + } else { + rclient.srem(keys.historyRangesSupport(), docId, callback) + } +} + module.exports = RedisManager module.exports.promises = promisifyAll(RedisManager, { without: ['_deserializeRanges', '_computeHash'], diff --git a/services/document-updater/test/acceptance/js/RangesTests.js b/services/document-updater/test/acceptance/js/RangesTests.js index 0cb1bfbd30..424ea7e81e 100644 --- a/services/document-updater/test/acceptance/js/RangesTests.js +++ b/services/document-updater/test/acceptance/js/RangesTests.js @@ -441,88 +441,85 @@ describe('Ranges', function () { lines: ['aaa', 'bbb', 'ccc', 'ddd', 'eee'], } - DocUpdaterClient.enableHistoryRangesSupport(this.doc.id, (error, res) => { - if (error) { + MockWebApi.insertDoc(this.project_id, this.doc.id, { + lines: this.doc.lines, + version: 0, + historyRangesSupport: true, + }) + + DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => { + if (error != null) { throw error } - MockWebApi.insertDoc(this.project_id, this.doc.id, { - lines: this.doc.lines, - version: 0, - }) - DocUpdaterClient.preloadDoc(this.project_id, this.doc.id, error => { - if (error != null) { - throw error - } + this.id_seed_1 = 'tc_1' + this.id_seed_2 = 'tc_2' + this.id_seed_3 = 'tc_3' - this.id_seed_1 = 'tc_1' - this.id_seed_2 = 'tc_2' - this.id_seed_3 = 'tc_3' - - this.updates = [ - { - doc: this.doc.id, - op: [{ d: 'bbb', p: 4 }], - v: 0, - meta: { - user_id: this.user_id, - tc: this.id_seed_1, - }, + this.updates = [ + { + doc: this.doc.id, + op: [{ d: 'bbb', p: 4 }], + v: 0, + meta: { + user_id: this.user_id, + tc: this.id_seed_1, }, - { - doc: this.doc.id, - op: [{ d: 'ccc', p: 5 }], - v: 1, - meta: { - user_id: this.user_id, - tc: this.id_seed_2, - }, + }, + { + doc: this.doc.id, + op: [{ d: 'ccc', p: 5 }], + v: 1, + meta: { + user_id: this.user_id, + tc: this.id_seed_2, }, - { - doc: this.doc.id, - op: [{ d: 'ddd', p: 6 }], - v: 2, - meta: { - user_id: this.user_id, - tc: this.id_seed_3, - }, + }, + { + doc: this.doc.id, + op: [{ d: 'ddd', p: 6 }], + v: 2, + meta: { + user_id: this.user_id, + tc: this.id_seed_3, }, - ] + }, + ] - DocUpdaterClient.sendUpdates( - this.project_id, - this.doc.id, - this.updates, - error => { - if (error != null) { - throw error - } - setTimeout(() => { - DocUpdaterClient.getDoc( - this.project_id, - this.doc.id, - (error, res, data) => { - if (error != null) { - throw error - } - const { ranges } = data - const changeOps = ranges.changes - .map(change => change.op) - .flat() - changeOps.should.deep.equal([ - { d: 'bbb', p: 4 }, - { d: 'ccc', p: 5 }, - { d: 'ddd', p: 6 }, - ]) - done() - } - ) - }, 200) + DocUpdaterClient.sendUpdates( + this.project_id, + this.doc.id, + this.updates, + error => { + if (error != null) { + throw error } - ) - }) + setTimeout(() => { + DocUpdaterClient.getDoc( + this.project_id, + this.doc.id, + (error, res, data) => { + if (error != null) { + throw error + } + const { ranges } = data + const changeOps = ranges.changes + .map(change => change.op) + .flat() + changeOps.should.deep.equal([ + { d: 'bbb', p: 4 }, + { d: 'ccc', p: 5 }, + { d: 'ddd', p: 6 }, + ]) + done() + } + ) + }, 200) + } + ) }) }) + afterEach(function () { sandbox.restore() }) diff --git a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js index c86bd17e47..4ed4f929de 100644 --- a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js +++ b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js @@ -119,10 +119,6 @@ module.exports = DocUpdaterClient = { ) }, - enableHistoryRangesSupport(docId, cb) { - rclient.sadd(keys.historyRangesSupport(), docId, cb) - }, - preloadDoc(projectId, docId, callback) { DocUpdaterClient.getDoc(projectId, docId, callback) }, diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index 173e9aeeeb..7e0e2fcea8 100644 --- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js +++ b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js @@ -834,8 +834,11 @@ describe('RedisManager', function () { this.logger.error.calledWith().should.equal(false) }) - it('should not add the document to the HistoryRangesSupport set in Redis', function () { - this.multi.sadd.should.not.have.been.calledWith('HistoryRangesSupport') + it('should remove the document from the HistoryRangesSupport set in Redis', function () { + this.rclient.srem.should.have.been.calledWith( + 'HistoryRangesSupport', + this.docId + ) }) it('should not store the resolved comments in Redis', function () { @@ -956,7 +959,7 @@ describe('RedisManager', function () { }) it('should add the document to the HistoryRangesSupport set in Redis', function () { - this.multi.sadd.should.have.been.calledWith( + this.rclient.sadd.should.have.been.calledWith( 'HistoryRangesSupport', this.docId )