535136 - Improve transition from single-client to multi-client mode

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mike Connor [:mconnor]]

Right now, single-client mode means 24h sync intervals, so once another client is added, data will be stale (or be fresh, and then grow stale over a day before the first client starts syncing on 5m/1h intervals).  This is not great, and we can improve this without blowing up our scaling model.  In an ideal world, we'd have server notifications, but scaling XMPP or similar is an unknown for us right now.

What we can do, in terms of solutions that shouldn't hurt much, but really improve the situation, is make single-client mode poll info/collections more aggressively, and then sync if there's changes to clientData.  If we do this hourly, or even on a 30m cycle (if it's in memcache, it's fairly cheap) we'll pick up mode changes pretty aggressively, and reduce the need for users to manually sync on the desktop.

On the subject of memcache, we really need bug 515088 in place before we can safely turn this on in production builds.

Comment 1

[Mike Connor [:mconnor]]

Attachment: in single-client mode, do a heartbeat to look for additional clients

Comment 2

[Ed Lee :Mardak]

Couldn't we just "sync" every hour as if we were in multi-desktop mode but not actually sync any engines until we pass a threshold? The threshold would probably just be a pref tracking when we last sycned in single-client mode.

Comment 3

[Mike Connor [:mconnor]]

We could, but that feels far less clean, since then we'd have to differentiate in sync() whether it's user or timer triggered.  We then also have to deal with the part where we're firing off sync start/finish notifications, and triggering UI indicators, much more often than we're actually syncing, leading to potential user confusion.

Comment 4

[Mike Connor [:mconnor]]

Attachment: ignore, wrong bug

Comment 5

[Ed Lee :Mardak]

Comment on attachment 433368 [details] [diff] [review]
ignore, wrong bug

Wrong bug ;)

Comment 6

[Ed Lee :Mardak]

(In reply to comment #3)
> We could, but that feels far less clean, since then we'd have to differentiate
> in sync() whether it's user or timer triggered.
We've done that in the past where UI bits pass in true to indicate it's a forced sync. Reusing sync would also make sure the heartbeat it does existing checkSync (loggedin? online? etc) and remoteSetup (correct version?) checks.

> We then also have to deal with
> the part where we're firing off sync start/finish notifications, and triggering
> UI indicators, much more often than we're actually syncing, leading to
> potential user confusion.
I thought there were some discussion of getting rid of the spinner/string changes as it's just distracting as it goes so fast anyway. Perhaps bug 546551? We could also just make it delayed update so that if it finishes before the delay, don't even bother changing it.

Comment 7

[Mike Connor [:mconnor]]

(In reply to comment #6)
> (In reply to comment #3)
> > We could, but that feels far less clean, since then we'd have to differentiate
> > in sync() whether it's user or timer triggered.
> We've done that in the past where UI bits pass in true to indicate it's a
> forced sync. Reusing sync would also make sure the heartbeat it does existing
> checkSync (loggedin? online? etc) and remoteSetup (correct version?) checks.

checkSync is reusable enough.  Why would we want to make the heartbeat do any of the remoteSetup stuff?

> > We then also have to deal with
> > the part where we're firing off sync start/finish notifications, and triggering
> > UI indicators, much more often than we're actually syncing, leading to
> > potential user confusion.
> I thought there were some discussion of getting rid of the spinner/string
> changes as it's just distracting as it goes so fast anyway. Perhaps bug 546551?
> We could also just make it delayed update so that if it finishes before the
> delay, don't even bother changing it.

But why? What's wrong with a separate timer and not conflating the two?  Overloading sync() doesn't seem to buy us anything useful...

Comment 8

[Ed Lee :Mardak]

Comment on attachment 433199 [details] [diff] [review]
in single-client mode, do a heartbeat to look for additional clients

(In reply to comment #7)
> But why? What's wrong with a separate timer and not conflating the two? 
> Overloading sync() doesn't seem to buy us anything useful...
Sync will probably want to do something similar to the heartbeat down the line of checking info/collections to detect if there are changes.

>+++ b/source/modules/service.js
>@@ -1041,16 +1043,57 @@ WeaveSvc.prototype = {
>+  _doHeartbeat: function WeaveSvc__doHeartbeat() {
>+    if (this._heartbeatTimer)
>+      this._heartbeatTimer.clear();
>+
>+    let info = new Resource(this.infoURL).get();
Do we need to lock the service on this network access? This can also throw on network failures.

>+    if (info.success) {
>+    }
>+    else {
>+      this._checkServerError(info);
Should this check for 401 cluster changes? _checkServerError only handles some 5xx responses. And this will set backoff, but do we want to enforce that for the heartbeat? Right now it only prevents syncing engines in sync(), so perhaps we don't need to enforce it here either?

Comment 9

[Ed Lee :Mardak]

Oh wait, sync triggers are cleared at the start of sync, so it'll also clear the heartbeat timer. And the heartbeat only gets scheduled if there's no backoff, yeah? I guess if the server is telling us the backoff, we probably don't need to schedule a heartbeat 1-hour after the backoff ends.

Comment 10

[Mike Connor [:mconnor]]

I think sync is already a big beast.  Duplicating a small chunk of logic is fine, if it avoids creating more complexity to sync(), and I'm actually wondering if we really just want to have heartbeat be a recurring slack timer that gets canceled when we hit sync.

Right now, heartbeat is simple and self-contained, and entirely irrelevant for numClients > 1, whereas if we try to over-generalize the logic, we probably have a more complex testing matrix.

Also, to reply to the bit about getting rid of sync UI in the statusbar, sure.  But any consumers observing start/finish will get bogus notifications, and logging looks weird, so.. meh.

Comment 11

[Mike Connor [:mconnor]]

Attachment: post-discussion, add better comments, tweak logging levels

Comment 12

[Ed Lee :Mardak]

Comment on attachment 433983 [details] [diff] [review]
post-discussion, add better comments, tweak logging levels

>+++ b/source/modules/service.js
>+  _doHeartbeat: function WeaveSvc__doHeartbeat() {
>+    let info = null;
Don't need to explicitly set to null; undefined works.
>+    if (info && info.success) {
>+    else {
>+      this._checkServerError(info);
info might be null/undefined so this would need to also check for != null. Maybe just put this whole block in the try/catch and leave the scheduleHeartbeat outside.

Comment 13

[Mike Connor [:mconnor]]

Attachment: final

Comment 14

[Mike Connor [:mconnor]]

http://hg.mozilla.org/labs/weave/rev/1e251085d6a7