⚡️ Avoid committing no-ops to the database #682
Conversation
alecgibson
force-pushed
the
avoid-committing-no-ops
branch
2 times, most recently
from
1466e83
to
714811b
Compare
alecgibson
force-pushed
the
avoid-committing-no-ops
branch
from
714811b
to
9318db8
Compare
lib/submit-request.js
Outdated
| // The op is a no-op, either because it was submitted as such, or - more | ||
| // likely - because it was transformed into one. Let's avoid committing it | ||
| // and tell the client. | ||
| return callback(request.noOpError()); |
There was a problem hiding this comment.
This check has to happen at some point after the op is transformed, and before the op is committed.
I don't think I have a strong opinion on when we return this, so long as it's at either one of these extremes(?).
The benefit of doing it just before commit() is that it's a bit more directly linked to the fact that commit is about to be skipped. It does mean that all the middleware will be fired, though (which may or may not be a good/expected thing?).
There was a problem hiding this comment.
After some discussion, we decided to move just after transform, before the "apply" hook.
The "commit" hook right now means that sharedb core is going to call the DB adapter's commit method, and leaving the no-op early return here would have meant a violation of that behavior. By moving it earlier, that behavior is preserved.
lib/client/doc.js
Outdated
There was a problem hiding this comment.
Should this be an opt-in feature behind a flag? I don't know if this should be considered a breaking change?
There was a problem hiding this comment.
Okay I wound up:
- hiding this behind a feature flag
- bumping the protocol to
1.2and adding a protocol check, so that old clients won't getERR_NO_OPerrors that they don't know how to handle
| @@ -35,6 +35,7 @@ ShareDBError.CODES = { | |||
| ERR_MAX_SUBMIT_RETRIES_EXCEEDED: 'ERR_MAX_SUBMIT_RETRIES_EXCEEDED', | |||
| ERR_MESSAGE_BADLY_FORMED: 'ERR_MESSAGE_BADLY_FORMED', | |||
| ERR_MILESTONE_ARGUMENT_INVALID: 'ERR_MILESTONE_ARGUMENT_INVALID', | |||
| ERR_NO_OP: 'ERR_NO_OP', | |||
There was a problem hiding this comment.
Should we add docs about the error?
alecgibson
force-pushed
the
avoid-committing-no-ops
branch
2 times, most recently
from
6e08880
to
bab5db1
Compare
alecgibson
force-pushed
the
avoid-committing-no-ops
branch
2 times, most recently
from
179a883
to
3eec5a0
Compare
alecgibson
force-pushed
the
client-protocol
branch
from
78c5f03
to
a7976d4
Compare
alecgibson
force-pushed
the
avoid-committing-no-ops
branch
from
3eec5a0
to
4e38759
Compare
alecgibson
force-pushed
the
client-protocol
branch
from
a7976d4
to
ad9aa7e
Compare
alecgibson
force-pushed
the
avoid-committing-no-ops
branch
from
4e38759
to
c752c4a
Compare
alecgibson
force-pushed
the
avoid-committing-no-ops
branch
5 times, most recently
from
5157251
to
d7a0921
Compare
This change checks for no-ops just before committing, and avoids writing
them to the database.
Motivation
----------
Sometimes we have situations where multiple clients try to make an
identical change to the same document at the same time (if multiple
clients are trying to keep two documents in sync, for example).
When this happens, this can result in a `O(n^2)` retry cascade, since
the requests will all keep retrying, with only a single request
succeeding at a time, even if those ops are all no-ops.
Solution
--------
This change adds a new special `ERR_NO_OP` error code. If the server
detects a no-op, it will skip committing, and early-return this error
code, which takes the request out of the retry queue.
The client then has some special handling for this case, where it will:
1. Fetch from remote to get up-to-date with any potential conflicting
ops
2. Then acknowledge the in-flight op, **without** bumping the version
(since the op wasn't committed)
Note that if `$fixup()` has been used, we **do not** early-return a
no-op error, even if the fixup results in a no-op, since the client
would be left in an inconsistent state without the fixup ops being
returned and applied.
alecgibson
force-pushed
the
avoid-committing-no-ops
branch
from
d7a0921
to
f91d42c
Compare
This change checks for no-ops just before committing, and avoids writing them to the database.
Motivation
Sometimes we have situations where multiple clients try to make an identical change to the same document at the same time (if multiple clients are trying to keep two documents in sync, for example).
When this happens, this can result in a
O(n^2)retry cascade, since the requests will all keep retrying, with only a single request succeeding at a time, even if those ops are all no-ops.Solution
This change adds a new special
ERR_NO_OPerror code. If the server detects a no-op, it will skip committing, and early-return this error code, which takes the request out of the retry queue.The client then has some special handling for this case, where it will:
Note that if
$fixup()has been used, we do not early-return a no-op error, even if the fixup results in a no-op, since the client would be left in an inconsistent state without the fixup ops being returned and applied.Depends on:
Agent#684