610923 - Allow engines to bypass the tracker for certain or all changed items

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

For bug 609395 we want to be able to bypass the tracker for collecting the list of changed IDs since we can do that for most data types based on timestamps now. Other datatypes don't even need to track that information persistently (e.g. tabs) or can use other means (e.g. prefs). This way we avoid disk I/O in most cases.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

SyncEngine now uses the getChangedIDs() method to get all changed IDs before a sync. By default this still goes through the tracker, but engines can override this method to do queries based on timestamps, or to bypass the tracker entirely.

SyncEngine now also exposes a lastSyncLocal property that keeps the local timestamp of the last sync so that engines can do timestamp based queries.

Comment 2

[Mike Connor [:mconnor]]

Comment on attachment 489408 [details] [diff] [review]
v1


>-        // Record the modified time of the upload
>+        // Update server timestamp from the upload.
>         let modified = resp.headers["x-weave-timestamp"];
>         if (modified > this.lastSync)
>           this.lastSync = modified;
> 
>+        // Update local timestamp.
>+        this.lastSyncLocal = Date.now();
>+

You'll need to cache the local time from when you retrieved the records to sync, but set it here, otherwise anything done during a long sync won't be synced.  Especially for syncs of >100 records, we'll run into problems.

>-    // Mark failed WBOs as changed again so they are reuploaded next time.
>+    // Mark failed WBOs as changed so they are might be reuploaded next time

??

>   _resetClient: function SyncEngine__resetClient() {
>     this.resetLastSync();
>+    this.lastSyncLocal = 0;

Why not make this part of resetLastSync?

I'd like to set these addressed in a new patch.

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #2)
> >+        // Update local timestamp.
> >+        this.lastSyncLocal = Date.now();
> >+
> 
> You'll need to cache the local time from when you retrieved the records to
> sync, but set it here, otherwise anything done during a long sync won't be
> synced.  Especially for syncs of >100 records, we'll run into problems.

You're right. I wonder whether we handle this case correctly so far (we iterate over changedIDs while they might be changing...). Anyway, to do this correctly we should remember the timestamp and set lastSyncLocal after *all* uploads so that it doesn't get set if one of the upload fails.

> >   _resetClient: function SyncEngine__resetClient() {
> >     this.resetLastSync();
> >+    this.lastSyncLocal = 0;
> 
> Why not make this part of resetLastSync?

resetLastSync seems only to exist because lastSync because it was a number in some ancient Weave version and now is a string. Hmm, actually, the lastSync setter already accommodates for that so, yeah, why not make this part of resetLastSync...

Comment 4

[Philipp von Weitershausen [:philikon]]

Attachment: v2

Changes from v1:

* Set lastSyncLocal after all records have uploaded successfully. Use the timestamp from when getChangedIDs() was called (this addresses mconnor's review comment).

* Clear the tracker immediately after getChangedIDs() but roll back when sync aborts due to an error. That way we handle objects that are tracked while a sync is going on in an unambiguous way.

* Reset lastSyncLocal in resetSync() as well (this addresses mconnor's review comment).

Comment 5

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/services/fx-sync/rev/1cfc623ef837