Initial implementation of the Sync Manager #1447

thomcc · 2019-07-23T03:10:29Z

Fixes #1382, although this might just become a rolling PR, since I don't think we want to land broken bindings.

See also mozilla-mobile/android-components#2727

This also required some number of changes to various other pieces in support.

The biggest differences between this and the API @mhammond proposed in our docs:

set_places/set_logins needs to be called to pass the handles of the places/logins items.
Swift support is entirely out of scope, and so most of the existing sync infrastructure is just left alone.
I've added a SyncReason.ENABLED_CHANGED for cases where we're triggering a sync of no engines just to update enabled.
wipe is gone, for now, as it's not clear when 'delete all local data of all engines' would be used in such a way that going through the sync manager is desired. (Just delete the database files, TBH).

Compared to #1208 (which is... surprisingly still not landed...), I still only return the declined list over the FFI. It's not 100% clear that I should expose anything more than that?

This is a draft PR, but I'd like feedback on the (Kotlin parts of the) API so far. Note that I'll be deprecating (eventually removing the existing kotlin sync APIs), but that's not yet reflected here.

r? @Amejia481, @grigoryk, @mhammond

Pull Request checklist

Quality: This PR builds and tests run cleanly
- cargo test --all produces no test failures
- cargo clippy --all --all-targets --all-features runs without emitting any warnings
- cargo fmt does not produce any changes to the code
- ./gradlew ktlint detekt runs without emitting any warnings
- swiftformat --swiftversion 4 megazords components/*/ios && swiftlint runs without emitting any warnings or producing changes
- Note: For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Amejia481 · 2019-07-24T01:53:20Z

components/sync_manager/android/src/main/java/mozilla/appservices/syncmanager/Errors.kt

+ * Base class for sync manager errors. Generally should not
+ * have concrete instances.
+ */
+open class SyncManagerException(msg: String) : Exception(msg)


Maybe sealed classes could be an alternative here.

thomcc · 2019-08-29T23:22:53Z

2 things not fully finished

Declined handling not 100% threaded through everything.
Sync manager forces use of both places and logins, instead of allowing them to be optional.

First of these I'll finish tomorrow ASAP (had more time in meetings than anticipated today) which should unblock @grigoryk's ability to test against this branch. 2nd I'll finish before landing but it shouldn't impact the overall shape of things.

As-is this probably should unblock @linacambridge's ability to integrate clients engine.

thomcc · 2019-09-03T18:11:21Z

Ignore the dep summary failure, that's because this currently forces lockwise to contain the places deps, which i'll fix after I do a little testing to make sure this all works as intended.

mhammond

This is looking good. However, I'd like to better understand why "accepted" isn't supported? I'd really like to avoid having Fenix spend effort on something we want to go away in the short term (and I really don't want "accepted" to not happen short term). It's particularly important IMO because IIUC we will be implementing "choose what to sync" sooner rather than later for Fenix, and because the set of engines supported is different to desktop we'll have a problem. IOW, we need to implement support for "accepted" on desktop ASAP rather than deferring the implementation on Fenix!)

(I'm fine with the spelling of "accepted" in the RFC not being exactly how it is implemented - if you think that's necessary, let's discuss it!)

mhammond · 2019-09-05T01:24:10Z

components/sync15/src/state.rs

@@ -87,6 +84,34 @@ pub struct GlobalState {
    pub keys: EncryptedBso,
 }

+fn same_declined(a: &[String], b: &[String]) -> bool {


While this looks correct, does a unit test for it make sense?

mhammond · 2019-09-05T01:40:51Z

components/sync15/src/state.rs

+
+impl MetaGlobalRecord {
+    // TODO: this function needs to change once we understand 'accepted'.
+    pub fn update_declined_engines(&mut self, changes: &HashMap<String, bool>) -> bool {


Is this better named "maybe_update_declined"? Either way, a comment explaining the bool return probably makes sense.

mhammond · 2019-09-05T01:56:12Z

components/sync15/src/status.rs

@@ -66,6 +66,9 @@ pub struct SyncResult {
    /// The general health.


does the comment at the top of this struct need updating?

mhammond · 2019-09-05T02:04:26Z

components/sync_manager/README.md

+crate, and in some cases stretches (or arguably breaks) the abstraction barrier
+that `sync15` puts up.
+
+Unfortunately, places/logins/etc depend on sync15 directly, and so to prevent a


It seems like some of the issue here WRT stretching/breaking the abstractions is keeping sync_multiple in sync15, but logins (and even places) probably doesn't actually need that?

components/sync_manager/ffi/src/lib.rs

components/sync_manager/src/ffi.rs

mhammond · 2019-09-05T02:13:50Z

components/sync_manager/src/manager.rs

+pub struct SyncManager {
+    mem_cached_state: Option<MemoryCachedState>,
+    places: Weak<PlacesApi>,
+    logins: Weak<Mutex<PasswordEngine>>,


The fact logins has a Mutex but PlacesApi doesn't seems like there's some inconsistency here that should be abstracted away lower?

(The fact we can't just work with traits surprises me too, but I guess there's a good reason hidden away somewhere)

components/sync_manager/src/manager.rs

linabutler

This is looking great, and I ❤️ the sync_multiple refactor, that function was getting really long and complicated. I also wouldn't mind seeing this again with accepted, but I left a few comments and questions in the meantime!

components/logins/ffi/src/lib.rs

components/sync15/src/client.rs

components/sync15/src/state.rs

components/sync15/src/sync_multiple.rs

components/sync15/src/state.rs

components/sync_manager/android/src/main/java/mozilla/appservices/syncmanager/Errors.kt

components/sync_manager/android/src/main/java/mozilla/appservices/syncmanager/RustError.kt

components/sync_manager/android/src/main/java/mozilla/appservices/syncmanager/SyncManager.kt

components/sync_manager/ffi/Cargo.toml

…ed before this is actually ready for review

… work

…sadly)

…of engines into the next states

thomcc · 2019-09-16T23:29:41Z

@linacambridge This should be ready for you to take another look I think?

linabutler

Looks great!

components/sync15/src/state.rs

components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt

components/sync15/src/client.rs

components/sync15/src/state.rs

components/sync15/src/sync_multiple.rs

components/sync_manager/src/manager.rs

thomcc · 2019-09-17T19:16:31Z

Will land once green

Amejia481 reviewed Jul 24, 2019

View reviewed changes

thomcc force-pushed the smanager branch 3 times, most recently from 9715fe7 to 084d4c9 Compare July 29, 2019 19:55

thomcc force-pushed the smanager branch 3 times, most recently from 78f318b to b6fd39d Compare August 29, 2019 23:06

This was referenced Aug 29, 2019

Sync Manager: Support for sync interruption #1684

Closed

Sync Manager: Handle "accepted" list in addition to declined #1685

Closed

thomcc force-pushed the smanager branch from b6fd39d to dbbcb7d Compare August 29, 2019 23:20

thomcc force-pushed the smanager branch 2 times, most recently from 95b9581 to 9f84c35 Compare September 3, 2019 17:49

thomcc changed the title ~~Initial implementation of the Kotlin Sync Manager API~~ Initial implementation of the Sync Manager Sep 3, 2019

thomcc requested a review from linabutler September 3, 2019 17:54

grigoryk mentioned this pull request Sep 3, 2019

Integrate appservices SyncManager mozilla-mobile/android-components#2727

Closed

mhammond reviewed Sep 5, 2019

View reviewed changes

thomcc force-pushed the smanager branch from 936a58b to f97e44e Compare September 6, 2019 18:33

linabutler reviewed Sep 11, 2019

View reviewed changes

Thom Chiovoloni added 10 commits September 16, 2019 15:06

Initial implementation of the Kotlin Sync Manager API

9f6fced

Fix linter errors

5578dda

Clean up commits for sync manager work, a few things still not finish…

f8a5a57

…ed before this is actually ready for review

Fix clippy issue

cd0605e

Thread engine state enough through the state machine to unblock other…

7a38cbc

… work

Remove unnecessary lazy_static use because it bugged me

0f176fe

Properly handle reuploading meta/global

7e70276

Actually skip declined engines

838157c

Sync manger: Ensure we actually pass declined back to the caller

532bf95

Correctly handle backoff

360b194

Thom Chiovoloni added 7 commits September 16, 2019 15:06

Fix flipped condition in sync manager, and add more logging

f04c09a

Clean up sync_multiple, properly handle declined (still no accepted, …

69f6b5e

…sadly)

Use the correct server timestamp for reuploading meta/global

a9351b8

Fix protobuf serializatoin of persistedState

d2bbfda

Fix and make more explicit how we transform the current known states …

1d3f93e

…of engines into the next states

Remove wrong comment

b456f2d

Respond to review feedback

4c10f94

thomcc force-pushed the smanager branch from c2347fb to 34e25ce Compare September 16, 2019 22:11

Remove sync_manager from lockbox megazord

ef62758

thomcc force-pushed the smanager branch from 34e25ce to ef62758 Compare September 16, 2019 22:13

Changelog entry for sync manager

6c92e84

linabutler approved these changes Sep 17, 2019

View reviewed changes

Respond to review feedback

4a4b09a

thomcc merged commit 433e00a into master Sep 17, 2019

linabutler deleted the smanager branch September 17, 2019 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of the Sync Manager #1447

Initial implementation of the Sync Manager #1447

thomcc commented Jul 23, 2019 •

edited

Amejia481 Jul 24, 2019

thomcc commented Aug 29, 2019

thomcc commented Sep 3, 2019

mhammond left a comment

mhammond Sep 5, 2019

mhammond Sep 5, 2019

mhammond Sep 5, 2019

mhammond Sep 5, 2019

mhammond Sep 5, 2019

linabutler left a comment

thomcc commented Sep 16, 2019

linabutler left a comment

thomcc commented Sep 17, 2019

		@@ -66,6 +66,9 @@ pub struct SyncResult {
		/// The general health.

Initial implementation of the Sync Manager #1447

Initial implementation of the Sync Manager #1447

Conversation

thomcc commented Jul 23, 2019 • edited

Pull Request checklist

Amejia481 Jul 24, 2019

Choose a reason for hiding this comment

thomcc commented Aug 29, 2019

thomcc commented Sep 3, 2019

mhammond left a comment

Choose a reason for hiding this comment

mhammond Sep 5, 2019

Choose a reason for hiding this comment

mhammond Sep 5, 2019

Choose a reason for hiding this comment

mhammond Sep 5, 2019

Choose a reason for hiding this comment

mhammond Sep 5, 2019

Choose a reason for hiding this comment

mhammond Sep 5, 2019

Choose a reason for hiding this comment

linabutler left a comment

Choose a reason for hiding this comment

thomcc commented Sep 16, 2019

linabutler left a comment

Choose a reason for hiding this comment

thomcc commented Sep 17, 2019

thomcc commented Jul 23, 2019 •

edited