-
Notifications
You must be signed in to change notification settings - Fork 453
Add Milestone Snapshots #236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
1adbe74
8f72522
10902b7
e85d59e
37c0797
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
var DB = require('./index'); | ||
var Snapshot = require('../snapshot'); | ||
var ShareDBError = require('../error'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the refactor, this require is now unused and can be removed. |
||
|
||
// In-memory ShareDB database | ||
// | ||
|
@@ -48,6 +49,7 @@ MemoryDB.prototype.commit = function(collection, id, op, snapshot, options, call | |
if (err) return callback(err); | ||
err = db._writeSnapshotSync(collection, id, snapshot); | ||
if (err) return callback(err); | ||
|
||
var succeeded = true; | ||
callback(null, succeeded); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
var emitter = require('../emitter'); | ||
|
||
module.exports = MilestoneDB; | ||
function MilestoneDB(options) { | ||
emitter.EventEmitter.call(this); | ||
|
||
// The interval at which milestone snapshots should be saved | ||
this.interval = options && options.interval; | ||
} | ||
emitter.mixin(MilestoneDB); | ||
|
||
MilestoneDB.prototype.close = function(callback) { | ||
if (callback) callback(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The base and memory DB implementations aren't super-consistent about ensuring their callbacks are async, but for these two new milestone classes, I think it'd be good to consistently There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine by me. I was unsure why I'd seen so many There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yup, that's exactly it! |
||
}; | ||
|
||
/** | ||
* Fetch a milestone snapshot from the database | ||
* @param {string} collection - name of the snapshot's collection | ||
* @param {string} id - ID of the snapshot to fetch | ||
* @param {number} version - the desired version of the milestone snapshot. The database will return | ||
* the most recent milestone snapshot whose version is equal to or less than the provided value | ||
* @param {Function} callback - a callback to invoke once the snapshot has been fetched. Should have | ||
* the signature (error, snapshot) => void; | ||
*/ | ||
MilestoneDB.prototype.getMilestoneSnapshot = function (collection, id, version, callback) { | ||
callback(null, undefined); | ||
}; | ||
|
||
/** | ||
* @param {string} collection - name of the snapshot's collection | ||
* @param {Snapshot} snapshot - the milestone snapshot to save | ||
* @param {Function} callback (optional) - a callback to invoke after the snapshot has been saved. | ||
* Should have the signature (error, wasSaved) => void; | ||
*/ | ||
MilestoneDB.prototype.saveMilestoneSnapshot = function (collection, snapshot, callback) { | ||
var saved = false; | ||
if (callback) return callback(null, saved); | ||
this.emit('save', saved, collection, snapshot); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
var MilestoneDB = require('./index'); | ||
|
||
/** | ||
* In-memory ShareDB milestone database | ||
* | ||
* Milestone snapshots exist to speed up Backend.fetchSnapshot by providing milestones | ||
* on top of which fewer ops can be applied to reach a desired version of the document. | ||
* This very concept relies on persistence, which means that an in-memory database like | ||
* this is in no way appropriate for production use. | ||
* | ||
* The main purpose of this class is to provide a simple example of implementation, | ||
* and for use in tests. | ||
*/ | ||
module.exports = MemoryMilestoneDB; | ||
function MemoryMilestoneDB(options) { | ||
MilestoneDB.call(this, options); | ||
|
||
// Map form collection name -> doc id -> array of milestone snapshots | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo, |
||
this._milestoneSnapshots = {}; | ||
} | ||
|
||
MemoryMilestoneDB.prototype = Object.create(MilestoneDB.prototype); | ||
|
||
MemoryMilestoneDB.prototype.getMilestoneSnapshot = function (collection, id, version, callback) { | ||
var milestoneSnapshots = this._getMilestoneSnapshotsSync(collection, id); | ||
|
||
let milestoneSnapshot; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Server-only code could certainly start using ES6 features, as the oldest supported version of Node is 6 LTS. It'd be nice to use Map for higher performance than object properties. However, it isn't always obvious what's server-only vs client-only vs shared. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha whoops, yeah that was just a force of habit from my day-to-day work. I thought this would get picked up by the linter? (It certainly had a go at me for using the spread operator) |
||
for (var i = 0; i < milestoneSnapshots.length; i++) { | ||
var nextMilestoneSnapshot = milestoneSnapshots[i]; | ||
if (nextMilestoneSnapshot.v <= version || version === null) { | ||
milestoneSnapshot = nextMilestoneSnapshot; | ||
} else { | ||
break; | ||
} | ||
} | ||
|
||
callback(null, milestoneSnapshot); | ||
}; | ||
|
||
MemoryMilestoneDB.prototype.saveMilestoneSnapshot = function (collection, snapshot, callback) { | ||
var saved = false; | ||
if (!snapshot) { | ||
if (callback) return callback(null, saved); | ||
this.emit('save', saved, collection, snapshot); | ||
} | ||
|
||
var milestoneSnapshots = this._getMilestoneSnapshotsSync(collection, snapshot.id); | ||
milestoneSnapshots.push(snapshot); | ||
milestoneSnapshots.sort(function (a, b) { | ||
return a.v - b.v; | ||
}); | ||
|
||
saved = true; | ||
if (callback) return callback(null, saved); | ||
this.emit('save', saved, collection, snapshot); | ||
}; | ||
|
||
MemoryMilestoneDB.prototype._getMilestoneSnapshotsSync = function (collection, id) { | ||
var collectionSnapshots = this._milestoneSnapshots[collection] || (this._milestoneSnapshots[collection] = {}); | ||
return collectionSnapshots[id] || (collectionSnapshots[id] = []); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
var MemoryMilestoneDB = require('./../lib/milestone-db/memory'); | ||
|
||
require('./milestone-db')({ | ||
create: function(options, callback) { | ||
if (typeof options === 'function') { | ||
callback = options; | ||
options = null; | ||
} | ||
|
||
var db = new MemoryMilestoneDB(options); | ||
callback(null, db); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit wary of using MemoryMilestoneDB as the default here. I do see that it doesn't store any snapshots unless it's constructed with
{interval: <integer>}
, so it won't accumulate snapshots in-memory by default.Posing a question for discussion - Thoughts on using the base MilestoneDB as the default instead, for virtually no overhead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was in two minds about this. I used
MemoryMilestoneDB
for consistency withMemoryDB
andMemoryPubSub
, but actuallyMemoryMilestoneDB
just acts like the dumbMilestoneDB
when initialised like this, so it's probably better to just use the base implementation. If you wanted to set an interval, you'd need to re-initialise it and explicitly pass it in anyway.I'll switch for the base implementation (which was written to be completely harmless anyway), and then it's super clear that nothing's happening by default.