Improve transition from single-client to multi-client mode

RESOLVED FIXED in 1.2

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mconnor, Assigned: mconnor)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
blocking-weave1.2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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.
Flags: blocking-weave1.1+
(Assignee)

Updated

8 years ago
Target Milestone: 1.1 → 1.2
(Assignee)

Comment 1

8 years ago
Created attachment 433199 [details] [diff] [review]
in single-client mode, do a heartbeat to look for additional clients
Attachment #433199 - Flags: review?(edilee)

Comment 2

8 years ago
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.
(Assignee)

Comment 3

8 years ago
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.
(Assignee)

Comment 4

8 years ago
Created attachment 433368 [details] [diff] [review]
ignore, wrong bug
Attachment #433199 - Attachment is obsolete: true
Attachment #433368 - Flags: review?(edilee)
Attachment #433199 - Flags: review?(edilee)
(Assignee)

Updated

8 years ago
Attachment #433199 - Attachment is obsolete: false
Attachment #433199 - Flags: review?(edilee)

Comment 5

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

Wrong bug ;)
(Assignee)

Updated

8 years ago
Attachment #433368 - Attachment description: better → ignore, wrong bug
Attachment #433368 - Attachment is obsolete: true
Attachment #433368 - Flags: review?(edilee)

Comment 6

8 years ago
(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.
(Assignee)

Comment 7

8 years ago
(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...
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][needs review Mardak]

Comment 8

8 years ago
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

8 years ago
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.
(Assignee)

Comment 10

8 years ago
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.
(Assignee)

Comment 11

8 years ago
Created attachment 433983 [details] [diff] [review]
post-discussion, add better comments, tweak logging levels
Attachment #433199 - Attachment is obsolete: true
Attachment #433983 - Flags: review?(edilee)
Attachment #433199 - Flags: review?(edilee)

Comment 12

8 years ago
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.
Attachment #433983 - Flags: review?(edilee) → review+

Updated

8 years ago
Whiteboard: [has patch][needs review Mardak] → [needs new patch][has review]
(Assignee)

Comment 13

8 years ago
Created attachment 434028 [details] [diff] [review]
final
Attachment #433983 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Whiteboard: [needs new patch][has review] → [has patch][has review]
(Assignee)

Comment 14

8 years ago
http://hg.mozilla.org/labs/weave/rev/1e251085d6a7
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Whiteboard: [has patch][has review]

Updated

8 years ago
Blocks: 546077

Updated

8 years ago
Blocks: 560887
You need to log in before you can comment on or make changes to this bug.