Closed Bug 1313890 Opened 5 years ago Closed 4 years ago
Wait to pull changes from the bookmarks tracker before uploading
Currently, the bookmarks engine calls `pullNewChanges` (or `pullAllChanges`, for the first sync) at the start of the sync. It uses this initial changeset to resolve conflicts (http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/services/sync/modules/engines.js#1332,1336-1337,1401,1403,1406,1443,1469). In bug 1258127, `PlacesSyncUtils.bookmarks.remove` and `touch` also return new change records that are merged into the initial changeset. The final merged set determines which bookmarks to upload. `remove` and `touch` both return changesets for every item with a change counter > 1. These are supersets of the initial changeset, because we're in the middle of a sync and haven't called `pushChanges` yet. We don't expect these methods to be called frequently: `remove` is called once at the end of `_processIncoming`, and `touch` is only called if we've modified a record locally that's deleted on the server. We also expect the number of change records to be small, so I think the inefficiency is acceptable. But it's redundant work that we can avoid if we wait to pull changes until we're actually ready to upload. It would also bring us closer to bug 1305563, where we can (eventually) apply buffered records, reconcile, and build an outgoing changeset in a single operation. This would probably involve overriding `_reconcile` into the bookmarks engine, and refactoring so that it doesn't depend on `_modified`.
(In reply to Kit Cambridge [:kitcambridge] from comment #0) > `remove` and `touch` both return changesets for every item with a change > counter > 1. These are supersets of the initial changeset, because we're in > the middle of a sync and haven't called `pushChanges` yet. We don't expect > these methods to be called frequently: `remove` is called once at the end of > `_processIncoming`, and `touch` is only called if we've modified a record > locally that's deleted on the server. We also expect the number of change > records to be small, so I think the inefficiency is acceptable. One way I considered at first: add a new sync status (SYNC_STATUS_RECONCILED) and source (SOURCE_SYNC_RECONCILE). Modifying a bookmark using that source would change its status to SYNC_STATUS_RECONCILED. `dedupe`, `remove`, and `touch` would then query for bookmarks WHERE syncChangeCounter >= 1 AND syncStatus = SYNC_STATUS_RECONCILED. The big downside of that approach is that it complicates change counter handling. We'd need to do something like `SET syncChangeCounter = syncChangeCounter + :delta, syncStatus = IFNULL(:newSyncStatus, syncStatus)`, and have a helper that returns SYNC_STATUS_RECONCILED or null, depending on the source. This feels like a weird, magical interaction between the source, change counter, and status. And, after all that, I think we'd still want this bug, anyway. So we'd introduce a lot of temporary complexity for rare cases. But maybe there's a simpler way. :-)
The mirror uses a temp table to stage outgoing bookmarks for upload.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1305563
You need to log in before you can comment on or make changes to this bug.