518273 - Need to handle not having an active node for Sync

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mike Connor [:mconnor]]

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.

Comment 1

[Mike Connor [:mconnor]]

Attachment: patch

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

Comment 2

[Mike Connor [:mconnor]]

Attachment: 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.

Comment 3

[Ed Lee :Mardak]

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

Comment 4

[Mike Connor [:mconnor]]

(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!

Comment 5

[Mike Connor [:mconnor]]

http://hg.mozilla.org/labs/weave/rev/93e57c1b5e52