If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Need to handle not having an active node for Sync

RESOLVED FIXED in 0.7

Status

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

People

(Reporter: mconnor, Assigned: mconnor)

Tracking

unspecified
Points:
---
Bug Flags:
blocking-weave1.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Bug 518271 means we sometimes won't get a node from a chknode call.  We need to handle this case and retry chknode after some timeout.  This will primarily be on new-user cases, but it's possible that we'll use this if a master/slave fail in tandem, or if we're migrating users to a different node, etc.

An empty node will effectively be the same as disabling sync.
Flags: blocking-weave1.0+
(Assignee)

Comment 1

8 years ago
Created attachment 403362 [details] [diff] [review]
patch

Needs sane UI handling, but this should work for the backend.
(Assignee)

Comment 2

8 years ago
Created attachment 403629 [details] [diff] [review]
better

Some of this isn't... awesome, but it works as intended.  The 10 minute delay was suggested by Toby as a reasonable value, and I'm entirely content to poll every 10 minutes in the name of maximizing speed of uptake.
Attachment #403362 - Attachment is obsolete: true
Attachment #403629 - Flags: review?(edilee)

Comment 3

8 years ago
Comment on attachment 403629 [details] [diff] [review]
better

>+++ b/source/chrome/content/sync.js
>@@ -175,27 +175,42 @@ WeaveWindow.prototype = {
>     // Clear out sync failures on a successful sync
>     else
>       Weave.Notifications.removeAll(title);
> 
>+    if (this._wasDelayed && Weave.Service.status.sync != Weave.NO_SYNC_NODE_FOUND) {
>+      title = this._stringBundle.getString("error.sync.no_node_found.title");
>+      Weave.Notifications.removeAll(title);
>+      this._wasDelayed = false;
Probably can get rid of the _wasDelayed flag and just check for NO_SYNC_NODE_FOUND on a status = true (use the same else block above)

>   onNotificationAdded: function WeaveWin_onNotificationAdded() {
>     document.getElementById("sync-notifications-button").hidden = false;
>+    let notifications = Weave.Notifications.notifications;
>+    let priority = 0;
>+    for (let i = 0;i < notifications.length;i++)
>+      priority = Math.max(notifications[i].priority, priority);
I suppose if you really wanted to..
Math.max.apply(Math, Weave.Notifications.notifications.map(function(i) i.priority))

>+++ b/source/modules/service.js
>@@ -537,18 +540,21 @@ WeaveSvc.prototype = {
>   _verifyLogin: function _verifyLogin()
>     this._catch(this._notify("verify-login", "", function() {
>       // Make sure we have a cluster to verify against
>+      if (this.clusterURL == "" && !this._setCluster()) {
Share this check and set for verifyLogin and sync.

>@@ -1077,16 +1083,21 @@ WeaveSvc.prototype = {
>   sync: function WeaveSvc_sync(fullSync)
>     this._catch(this._lock(this._notify("sync", "", function() {
>     this.status.resetEngineStatus();
>+    if (this.clusterURL == "" && !this._setCluster()) {
>+      this._scheduleNextSync(10 * 60 * 1000);
>+      return;
>+    }
nit: Comments. Also, you'll hit some conflicts rebasing... maybe.

r=Mardak
Attachment #403629 - Flags: review?(edilee) → review+
(Assignee)

Comment 4

8 years ago
(In reply to comment #3)
> (From update of attachment 403629 [details] [diff] [review])
> >+++ b/source/chrome/content/sync.js
> >@@ -175,27 +175,42 @@ WeaveWindow.prototype = {
> >     // Clear out sync failures on a successful sync
> >     else
> >       Weave.Notifications.removeAll(title);
> > 
> >+    if (this._wasDelayed && Weave.Service.status.sync != Weave.NO_SYNC_NODE_FOUND) {
> >+      title = this._stringBundle.getString("error.sync.no_node_found.title");
> >+      Weave.Notifications.removeAll(title);
> >+      this._wasDelayed = false;
> Probably can get rid of the _wasDelayed flag and just check for
> NO_SYNC_NODE_FOUND on a status = true (use the same else block above)

discussed on IRC: this would mean we don't clear this notification if we get a node and then fails, and means we call removeAll roughly a million times more than needed ;)

> >   onNotificationAdded: function WeaveWin_onNotificationAdded() {
> >     document.getElementById("sync-notifications-button").hidden = false;
> >+    let notifications = Weave.Notifications.notifications;
> >+    let priority = 0;
> >+    for (let i = 0;i < notifications.length;i++)
> >+      priority = Math.max(notifications[i].priority, priority);
> I suppose if you really wanted to..
> Math.max.apply(Math, Weave.Notifications.notifications.map(function(i)
> i.priority))

I don't, unless it's vastly more performant.

> >+++ b/source/modules/service.js
> >@@ -537,18 +540,21 @@ WeaveSvc.prototype = {
> >   _verifyLogin: function _verifyLogin()
> >     this._catch(this._notify("verify-login", "", function() {
> >       // Make sure we have a cluster to verify against
> >+      if (this.clusterURL == "" && !this._setCluster()) {
> Share this check and set for verifyLogin and sync.

Will file a followup on cleaning this up, we don't need a fourth *Cluster method for now though. :)

> >@@ -1077,16 +1083,21 @@ WeaveSvc.prototype = {
> >   sync: function WeaveSvc_sync(fullSync)
> >     this._catch(this._lock(this._notify("sync", "", function() {
> >     this.status.resetEngineStatus();
> >+    if (this.clusterURL == "" && !this._setCluster()) {
> >+      this._scheduleNextSync(10 * 60 * 1000);
> >+      return;
> >+    }
> nit: Comments. Also, you'll hit some conflicts rebasing... maybe.

Will add some useful comments and push, thanks!
(Assignee)

Comment 5

8 years ago
http://hg.mozilla.org/labs/weave/rev/93e57c1b5e52
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
No longer blocks: 518271
You need to log in before you can comment on or make changes to this bug.