🗃 Add _create metadata to snapshots to avoid getCommittedOpVersion()
#659
Conversation
| @@ -290,6 +299,20 @@ SubmitRequest.prototype._shouldSaveMilestoneSnapshot = function(snapshot) { | |||
| return this.saveMilestoneSnapshot; | |||
| }; | |||
|
|
|||
| SubmitRequest.prototype._fetchCreateOpVersion = function(callback) { | |||
| var create = this.snapshot.m._create; | |||
There was a problem hiding this comment.
Note that this approach has one pitfall: it opens up a new corner case where
an op may be incorrectly rejected instead of acked:
- Client A submits Create 1
- Create 1 committed
- Client A disconnects before receiving the ack
- Client B submits and commits Delete, then Create 2
- Client A reconnects, and resubmits Create 1
- Create 1 doesn't match the current snapshot metadata for Create 2, so it's rejected with
DOC_ALREADY_CREATEDinstead of the non-fatalOP_ALREADY_SUBMITTED
I don't know how much we care about this? It definitely is a bug that didn't used to exist, but I'm not sure it's important enough to keep leaning on getCommittedOpVersion() all the time?
In my personal opinion, if two clients are creating/deleting/recreating the same Doc in such a close span of time, errors are bound to happen anyway, since creates/deletes don't have great transformation behaviour. And if this is an expected use case, consumers should already have robust error-handling in place for this sort of conflict.
There was a problem hiding this comment.
Per discussion, this should be a vanishingly small edge case in practice, and Client A reconnecting may run into other errors while reconnecting anyways that would require it to re-initialize.
alecgibson
force-pushed
the
src-seq-create-meta
branch
from
9f08977 to
7ba7f0b
Compare
alecgibson
force-pushed
the
src-seq-create-meta
branch
from
7ba7f0b to
efabb26
Compare
test/client/submit.js
Outdated
| if (fields) fields.$submit = false; | ||
| getSnapshot.call(this, collection, id, fields, snapshotOptions, callback); |
There was a problem hiding this comment.
I'm a bit worried $submit has or in the future will have other side effects, so it'd be safer to manually strip the metadata from the resulting snapshot
| @@ -290,6 +299,20 @@ SubmitRequest.prototype._shouldSaveMilestoneSnapshot = function(snapshot) { | |||
| return this.saveMilestoneSnapshot; | |||
| }; | |||
|
|
|||
| SubmitRequest.prototype._fetchCreateOpVersion = function(callback) { | |||
| var create = this.snapshot.m._create; | |||
There was a problem hiding this comment.
Per discussion, this should be a vanishingly small edge case in practice, and Client A reconnecting may run into other errors while reconnecting anyways that would require it to re-initialize.
Supersedes #657 The `DB.getCommitedOpVersion()` function is only used [in one place][1]: in a corner case that differentiates between two "blind", versionless (created without first fetching) create op conflict cases: - a versionless create op that conflicts with another create op - a versionless create op that has been re-submitted because the connection was closed before the ack was received The first of these cases should result in an error, and the second is non-fatal and the error can be swallowed. At the moment, this differentiation is made using the special `DB.getCommittedOpVersion()` function, which - given an op with a `src` and `seq` combination - will return `null` if the op hasn't been committed, or its version number if it has. If the op has a committed version number, we know that the submit is a duplicate, and we can safely ignore it. This approach is problematic, because: - it [needs a whole op index][2] just to handle this niche corner case - the default behaviour [fetches **all** ops][3] unless the driver overrides this behaviour This change proposes that we actually don't need this function in most cases, and implements an alternative approach to differentiate between the above cases. When creating an snapshot, we store the `src`, `seq` and `v` of the op that was used to create it. If a conflicting `create` is received, we can then compare directly with this metadata, without needing to fetch anything extra from the database. This should work in the majority of cases, but won't work if the metadata is missing, which could happen if: - the snapshot is "old": it was created before this change - the driver doesn't support metadata (eg [Postgres][4]) In the case where metadata is unavailable, we fall back to the existing method, using `getCommittedOpVersion()`. However, if drivers support metadata, this should happen sufficiently infrequently that consumers could potentially remove the `src`/`seq`/`v` index with no noticeable performance penalty. [1]: https://github.com/share/sharedb/blob/7b20313ded4c302b9416cc6c7821694a7fa491b8/lib/submit-request.js#L112 [2]: share/sharedb-mongo#94 [3]: https://github.com/share/sharedb/blob/7b20313ded4c302b9416cc6c7821694a7fa491b8/lib/db/index.js#L69 [4]: https://github.com/share/sharedb-postgres/blob/499702acd478645bcc249fa50ba6fc066d257d04/index.js#L140
alecgibson
force-pushed
the
src-seq-create-meta
branch
from
efabb26 to
5e5f1b4
Compare
Supersedes #657
The
DB.getCommitedOpVersion()function is only used in one place: in a corner case that differentiates between two "blind", versionless (created without first fetching) create op conflict cases:The first of these cases should result in an error, and the second is non-fatal and the error can be swallowed.
At the moment, this differentiation is made using the special
DB.getCommittedOpVersion()function, which - given an op with asrcandseqcombination - will returnnullif the op hasn't been committed, or its version number if it has.If the op has a committed version number, we know that the submit is a duplicate, and we can safely ignore it.
This approach is problematic, because:
This change proposes that we actually don't need this function in most cases, and implements an alternative approach to differentiate between the above cases.
When creating an snapshot, we store the
src,seqandvof the op that was used to create it.If a conflicting
createis received, we can then compare directly with this metadata, without needing to fetch anything extra from the database.This should work in the majority of cases, but won't work if the metadata is missing, which could happen if:
In the case where metadata is unavailable, we fall back to the existing method, using
getCommittedOpVersion(). However, if drivers support metadata, this should happen sufficiently infrequently that consumers could potentially remove thesrc/seq/vindex with no noticeable performance penalty.