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

RFC: Sync manager #714

Merged
merged 1 commit into from May 29, 2019

Conversation

@mhammond
Copy link
Member

commented Feb 26, 2019

As promised, I wrote up an initial RFC for the sync-manager a few of us have been discussing. It's not complete, but there should be enough there to get initial feedback.

Rendered

@mhammond mhammond requested review from rfk, grigoryk, thomcc and linacambridge Feb 26, 2019

@rfk
Copy link
Member

left a comment

Thanks for kicking this off @mhammond!

Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
@thomcc
Copy link
Contributor

left a comment

First review pass. Looks like it's moving in the right direction but I have some concerns.

  • Callbacks are going to be tricky to make work due to ownership -- Even completely ignoring FFI issues.
  • Which token is the storage_token?
  • I think we probably need to flesh out how clients collection will be synced a little more.
  • Several other vague concerns...

P.S. I edited your initial PR comment to include the link to the rendered markdown doc, hope you don't mind.

Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
@linacambridge
Copy link
Member

left a comment

This is fantastic, @mhammond (and I ❤️ this format), thanks for starting the discussion!

Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
@mhammond

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

I've pushed a new revision which I hope addresses most of the comments above - see #5d6e33b for the diff, but the main changes are:

Main changes:

  • Clarified that the tokenserver token may fail due to either auth issues or due to normal expiry (but for now I'm still sticking with the manager handling both those cases.

  • Clarified some stuff around commands.

  • Removed all references to callbacks, and made the (brave?) assertion that a rust implemented Sync manager will only ever deal with rust implemented engines, and further, that the engines and sync manager must all be in the same library.

  • Removed all references to JSON

  • Tried to clarify comments around the "not necessarily a 1:1 relationship between engines and collections"

  • Added a few more things the manager will not do, and added some notes on telemetry.

  • Clarified that the "clients" collection will be managed by the manager rather than as a new engine.

  • Added the concept of a "prepare" function.

I'm going to resolve the conversations above which I hope I've suitably addressed - please open new conversations for what I've missed or made worse.

@jdragojevic jdragojevic added the ↕ Sync label Mar 4, 2019

@mhammond mhammond added the backlog label Mar 4, 2019

@rfk rfk added this to the Rustmarks milestone Mar 5, 2019

Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
@grigoryk

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

I left some inline comments.

Could you expand on how the proposed SyncManager will tie in with the individual storage layers? E.g. history has a "sync" method, and SyncManager has a sync(engines...) method. Will the rust syncmanager implementation invoke rust engines itself, passing in the state that it maintains?
It also wasn't entirely clear to me who's responsible for persisting that state. You mention that the state is FFI friendly to serve non-Rust implementations. I imagine in that case history/logins/etc storage library APIs will expose a "prepare" function over FFI, to set up the sync state, and the swift/js/whatever sync manager will be expected to call that as needed? And in the case of applications that just use the Rust implementation of the manager, they will not need to interact with prepare/sync functions exposed by the storage libs. Is that correct?

I'm assuming that Rust SyncManager will persist the state itself, since its APIs do not expose it.

@mhammond

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

I've pushed a new version which I hope addresses most of the previous comments. I'll re-flag everyone for review and setup a meeting to discuss this.

@mhammond mhammond requested review from rfk, linacambridge, thomcc and grigoryk Apr 29, 2019

@mhammond mhammond self-assigned this Apr 29, 2019


* There may be some cross-crate issues even for the Rust implemented engines.
Or more specifically, we'd like to avoid assuming any particular linkage or
packaging of Rust implemented engines.

This comment has been minimized.

Copy link
@rfk

rfk Apr 30, 2019

Member

I think we may be able to avoid or simplify this complication in an always-megazord world?

This comment has been minimized.

Copy link
@mhammond

mhammond Apr 30, 2019

Author Member

We'll usually need/want to ensure we have the right set of "instances" with references to each other - the existing singletons are all outside rust. This is certainly doable and not major though, so I'll tone that down in the doc.


* Manage the "clients" collection - we probably can't ignore this any longer,
especially for bookmarks (as desktop will send a wipe command on bookmark
restore, and things will "be bad" if we don't see that command).

This comment has been minimized.

Copy link
@rfk

rfk Apr 30, 2019

Member

Hrm...we will need to think carefully about how this will interact with device registration over in the FxA component. Will the client record need to contain the FxA device id like it does on desktop?

This comment has been minimized.

Copy link
@mhammond

mhammond Apr 30, 2019

Author Member

The client engine will want the FxA device ID, simply so it can populate fxaDeviceId. Desktop uses this for device names etc in various places.

It's probably ideal if we could use the fxa device ID as the client ID, which I'm leaning towards - in my mind, the only issue with that is that the FxA IDs are more ephemeral (ie, don't survive some password/account related events) - but is that actually a problem? We'd track this and clean up after ourselves (ie, we'd delete our old client record when we noticed the ID change, etc)

* Define a minimal "service state" so certain things can be coordinated with
the high-level component. Examples of these states are "everything seems ok",
"the service requested we backoff for some period", "an authentication error
occurred", and possibly others.

This comment has been minimized.

Copy link
@rfk

rfk Apr 30, 2019

Member

Does this "service state" get persisted, both in-memory and across restarts?

This comment has been minimized.

Copy link
@mhammond

mhammond Apr 30, 2019

Author Member

I saw this as being completely transient from the component's POV - the API never needs it back. The embedding app might want to have it stick around in memory, but I don't think it ever needs to be persisted.

This comment has been minimized.

Copy link
@linacambridge

linacambridge May 1, 2019

Member

I went back and forth on this after our meeting yesterday, but I like this approach. Keeping the service state coarse still lets the high-level sync manager recover, but avoids pushing complex error handling logic into the app. And, considering that the app will probably want to remember this state, anyway, it makes sense that we don't try to store it, too.

## Clients Engine

The clients engine includes some meta-data about each client. We've decided
we can't replace the clients engine with the FxA device record and we can't

This comment has been minimized.

Copy link
@rfk

rfk Apr 30, 2019

Member

For posterity: why not?

This comment has been minimized.

Copy link
@mhammond

mhammond Apr 30, 2019

Author Member

"Because the ID to too ephemeral"? :) As above, I'm rethinking that - it's a real PITA for client code to deal with them being different, so we should suck it up and see what breaks :)


* A *store* is the code which actually syncs. This is largely an implementation
detail. There may be multiple stores per engine (eg, the "history" engine
may have "history" and "forms" stores.)

This comment has been minimized.

Copy link
@rfk

rfk Apr 30, 2019

Member

nit: is it technically "places" and "forms" stores since we implement a single syncable-store abstraction for bookmarks and history?

Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
* Define a minimal "service state" so certain things can be coordinated with
the high-level component. Examples of these states are "everything seems ok",
"the service requested we backoff for some period", "an authentication error
occurred", and possibly others.

This comment has been minimized.

Copy link
@linacambridge

linacambridge May 1, 2019

Member

I went back and forth on this after our meeting yesterday, but I like this approach. Keeping the service state coarse still lets the high-level sync manager recover, but avoids pushing complex error handling logic into the app. And, considering that the app will probably want to remember this state, anyway, it makes sense that we don't try to store it, too.

Show resolved Hide resolved docs/rfcs/sync-manager.md Outdated
different state request on a different device? Is the state as last-known on
this device considered canonical?

To clarify, consider:

This comment has been minimized.

Copy link
@linacambridge

linacambridge May 1, 2019

Member

whispers Three-way merging. I'm joking a bit, but also...what if the sync manager persisted the list of declined engines, recorded local changes since the last sync, and merged them with the server's view once the network is back up? That does push some complexity for noting declined state changes up into the high-level manager, which could be an argument for having the sync manager manage its own storage.

As you say, though, it's an extreme edge case, and having the manager manage its own database is probably not worth the benefit of handling conflicting declined changes.

This comment has been minimized.

Copy link
@mhammond

mhammond May 7, 2019

Author Member

whispers Three-way merging.

We could do that - but should we?

Storing the transition is simple because it is a bool state - so just storing (name, new_state) seems enough - old_state can be implied. And we already store declined state, so adding this probably doesn't blow out "state budget" and force us into our own state storage.

It's more about user intent. It's like logins, where we could reconcile "device-a changed the username but not the password" and "device-b changed the password but not the username". We could reconcile and end up with both "new" values, but it seems unlikely to reflect user intent.

Similarly here - it's not clear to me "device-a declined bookmarks but left history enabled" but "device-b declined history and left bookmarks enabled" is best served by merging these rather than just taking the latest.

But yeah - "not clear" is the keyword here, so I'm erring on the side of simplicity - both in implementation and explanation - but I'm totally open to the fact I'm underthinking this :)

@linacambridge
Copy link
Member

left a comment

Merge away!

@mhammond mhammond marked this pull request as ready for review May 29, 2019

@mhammond mhammond force-pushed the sync-manager-rfc branch from db293c3 to c47ce0a May 29, 2019

@mhammond mhammond merged commit 07ec77a into master May 29, 2019

11 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
ci/circleci: Check Rust dependencies Your tests passed on CircleCI!
Details
ci/circleci: Check Rust formatting Your tests passed on CircleCI!
Details
ci/circleci: Check Swift formatting Your tests passed on CircleCI!
Details
ci/circleci: Deploy website Your tests passed on CircleCI!
Details
ci/circleci: Lint Rust with clippy Your tests passed on CircleCI!
Details
ci/circleci: Rust benchmarks Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - beta Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - stable Your tests passed on CircleCI!
Details
ci/circleci: Sync integration tests Your tests passed on CircleCI!
Details
ci/circleci: iOS build and test Your tests passed on CircleCI!
Details

A-S Sprint Planning board automation moved this from Planned for Current Iteration to Done May 29, 2019

@mhammond mhammond deleted the sync-manager-rfc branch May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.