Merge pull request #19480 from overleaf/jpa-fast-path-fetch-for-transform

[document-updater] avoid fetching updates to transform when up-to-date

GitOrigin-RevId: 7962d8903a7bc9b572d7c6adfd8f33ad36f30459
This commit is contained in:
Jakob Ackermann 2024-07-23 11:36:52 +02:00 committed by Copybot
parent 8e79d72cc2
commit 8c0a78c7e7
2 changed files with 53 additions and 26 deletions

View file

@ -17,6 +17,12 @@ const Keys = require('./UpdateKeys')
const RedisManager = require('./RedisManager')
const Errors = require('./Errors')
const TRANSFORM_UPDATES_COUNT_BUCKETS = [
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 15, 20, 25, 50, 75, 100,
// prepare buckets for full-project history/larger buffer experiments
150, 200, 300, 400,
]
module.exports = ShareJsDB = class ShareJsDB {
constructor(projectId, docId, lines, version) {
this.project_id = projectId
@ -31,11 +37,18 @@ module.exports = ShareJsDB = class ShareJsDB {
}
getOps(docKey, start, end, callback) {
if (start === end) {
if (start === end || (start === this.version && end === null)) {
const status = 'is-up-to-date'
Metrics.inc('transform-updates', 1, {
status: 'is-up-to-date',
status,
path: 'sharejs',
})
Metrics.histogram(
'transform-updates.count',
0,
TRANSFORM_UPDATES_COUNT_BUCKETS,
{ path: 'sharejs', status }
)
return callback(null, [])
}
@ -64,35 +77,27 @@ module.exports = ShareJsDB = class ShareJsDB {
} else {
if (ops.length === 0) {
status = 'fetched-zero'
} else {
status = 'fetched'
if (start === this.version && end === -1) {
// The sharejs processing is happening under a lock.
// this.version is the version as read from redis under lock.
// In case there are any new ops available, something bypassed the lock (or we overran it).
// In case there are no other ops available, something bypassed the lock (or we overran it).
logger.warn(
{
projectId,
docId,
start,
nOps: ops.length,
end,
timeSinceShareJsDBInit:
performance.now() - this.startTimeShareJsDB,
},
'concurrent update of docOps while transforming update'
'found zero docOps while transforming update'
)
}
} else {
status = 'fetched'
}
Metrics.histogram(
'transform-updates.count',
ops.length,
[
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 15, 20, 25, 50, 75, 100,
// prepare buckets for full-project history/larger buffer experiments
150,
200, 300, 400,
],
TRANSFORM_UPDATES_COUNT_BUCKETS,
{ path: 'sharejs', status }
)
}

View file

@ -24,7 +24,9 @@ describe('ShareJsDB', function () {
this.callback = sinon.stub()
this.ShareJsDB = SandboxedModule.require(modulePath, {
requires: {
'./RedisManager': (this.RedisManager = {}),
'./RedisManager': (this.RedisManager = {
getPreviousDocOps: sinon.stub(),
}),
'./Errors': Errors,
'@overleaf/metrics': {
inc: sinon.stub(),
@ -87,11 +89,31 @@ describe('ShareJsDB', function () {
return this.db.getOps(this.doc_key, this.start, this.end, this.callback)
})
it('should not talk to redis', function () {
this.RedisManager.getPreviousDocOps.should.not.have.been.called
})
return it('should return an empty array', function () {
return this.callback.calledWith(null, []).should.equal(true)
})
})
describe('with start == redis-version and end unset', function () {
beforeEach(function () {
const start = this.version
const end = null
this.db.getOps(this.doc_key, start, end, this.callback)
})
it('should not talk to redis', function () {
this.RedisManager.getPreviousDocOps.should.not.have.been.called
})
it('should return an empty array', function () {
this.callback.should.have.been.calledWith(null, [])
})
})
describe('with a non empty range', function () {
beforeEach(function () {
this.start = 35