Skip to content
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

Add skipPoll support for json1 ops, instead of throwing errors for them #137

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ericyhwang
Copy link
Contributor

Resolves #136

sharedb-mongo implements the optional skipPoll method as an optimization for query subscriptions, which lets it tell sharedb core to skip polling the DB if an op can't possibly affect a subscribed query.

The implementation had been specific to json0 ops, and it would throw when encountering a json1 op due to the different op format, causing polling to always be skipped with json1.

This change adds skipPoll support for json1 ops, in the same way as it's done for json0 ops:

  1. From the subscribed query, extract all top-level doc fields that are matched by the query.
  2. Then, iterate over the op components.
  3. If an op component changes the entire doc, then assume the query is affected and don't skip polling.
  4. If the op component affects a queried field in any way, then don't skip polling. Otherwise, keep checking op components.

The implementation is a bit trickier for json1 ops. In contrast to json0, where an op component's subpath is present in a p path-segment array, json0 uses bare strings to indicate a "field descent", and a field descent from root level could be in different places:

// Entire op under single top-level field, in this case setting doc.a.x and doc.a.y
const op1 = ['a', ['x', {i:1}], ['y', {i:2}]];

// Affecting two top-level fields, in this case moving doc.x to doc.z
const op2 = [['x', {p:0}], ['z', {d:0}]];

// Field descent after a whole-doc change
// In this case, initialize the whole doc to `{tags: []}`, then insert 'rock' into the tags array.
// The whole-doc insert means the later field insert doesn't matter for skipPoll.
const op3 = [{i:{tags:[]}}, 'tags', 0, {i:'rock'}];

@ericyhwang
Copy link
Contributor Author

ericyhwang commented Jan 10, 2023

Bah, running into the done() called multiple times test flakiness again :(

@coveralls
Copy link

coveralls commented Jan 10, 2023

Coverage Status

Coverage: 92.832% (-0.02%) from 92.848% when pulling 67f6f1c on json1-skipPoll into e72c6bb on master.

@ericyhwang
Copy link
Contributor Author

  • Change so skipPoll will return false for unknown op structures, causing sharedb core to poll in those cases
  • Sanity check text-unicode and text-tp2

} else if (fields[component.p[0]]) {
return true;
}
} else if (typeof component === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check would break with eg text-unicode with a query set up on the root-level _data field (the default field if your snapshot isn't an object)

}
if (fields[descendant[0]]) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add an else statement here that captures types that are neither json0 nor json1 and default to polling.

@@ -1,4 +1,5 @@
var expect = require('chai').expect;
var json1 = require('ot-json1');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for rich-text (and any other types we want to check) please.

// First string in top-level array means all subsequent operations will be
// underneath that top-level field, no need to continue iterating.
return fields[component];
} else if (hasOwnProperty(component, 'p') ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I wonder if we're trying to do too much with heuristics here? It's really hard to determine if an op is a given type, and we may do a bunch of work we don't need to if a certain collection never submits JSON ops (this is the mainline case for our organisation!).

I wonder if this belongs as some sort of helper function plugin exported by this library, which consumers need to enable using ShareDB's skipPoll option.

So given a helper function shouldSkipJson1Poll(), consumer code might look something like:

const backend = new Backend({
  skipPoll: (collection, id, op, query) => {
    // Let's assume this is easy to determine for a consumer. Each
    // collection probably has a set type that never changes,
    // especially if we're querying!
    if (!isJson1Collection(collection)) return false;
    return shouldSkipJson1Poll(op, query);
  },
});

The advantages of this:

  • massively reduces sharedb-mongo's concern leakiness
  • we don't need to use heuristics to determine what types ops are
  • keeps the core of sharedb-mongo type-agnostic
  • we can have a "poll by default" fallback, so polling always works correctly

Disadvantages:

  • effort for consumers
  • obscure optimisation config that many consumers may never find or enable, and pay a performance penalty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ottypes/json1 support
3 participants