Last Comment Bug 691663 - SyncScheduler should obey backoffInterval at all times
: SyncScheduler should obey backoffInterval at all times
Status: VERIFIED FIXED
[verified in services][qa!]
: verified-aurora, verified-beta
Product: Cloud Services
Classification: Client Software
Component: Firefox Sync: Backend (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla10
Assigned To: Philipp von Weitershausen [:philikon]
:
Mentors:
Depends on: 691612 693413
Blocks: 692006
  Show dependency treegraph
 
Reported: 2011-10-03 21:04 PDT by Philipp von Weitershausen [:philikon]
Modified: 2011-12-28 15:49 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
affected
verified
verified
verified


Attachments
v1 (13.85 KB, patch)
2011-10-04 19:49 PDT, Philipp von Weitershausen [:philikon]
rnewman: review+
Details | Diff | Review
v2 (17.28 KB, patch)
2011-10-04 20:51 PDT, Philipp von Weitershausen [:philikon]
rnewman: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Review
patch for beta and release (14.70 KB, patch)
2011-10-05 16:54 PDT, Philipp von Weitershausen [:philikon]
asa: approval‑mozilla‑beta+
christian: approval‑mozilla‑release+
Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2011-10-03 21:04:19 PDT
These SyncScheduler methods don't always or at all obey backoffInterval:

* scheduleAtInterval: it does some weird thing with the sync error count and SyncScheduler.
* scheduleNextSync: when you pass a number in for 'interval', backoffInterval gets ignored.
* 'back' observer: it calls sync directly (via Utils.nextTick), it should go through scheduleNextSync, provided that one is fixed to obey backoffInterval

(Note that all but the last behaviour was carried over when SyncScheduler was broken out of Service.)
Comment 1 Philipp von Weitershausen [:philikon] 2011-10-04 19:49:51 PDT
Created attachment 564743 [details] [diff] [review]
v1

Make sure we always adhere to backoffInterval in scheduleNextSync() and in the back-from-idle observer that triggers a sync.
Comment 2 Richard Newman [:rnewman] 2011-10-04 19:57:49 PDT
Comment on attachment 564743 [details] [diff] [review]
v1

Review of attachment 564743 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray for pair programming!

::: services/sync/tests/unit/test_syncscheduler.js
@@ +204,5 @@
>    run_next_test();
>  });
>  
> +add_test(function test_scheduleNextSync_noBackoff() {
> +  _("scheduleNextSync() will use the current syncInterval if no interval is provided and ignore scheduling requests for larger intervals that the current one.");

_("If no interval is provided, scheduleNextSync() will use the current syncInterval. " +
  "Scheduling requests for intervals larger than the current one will be ignored.");

@@ +243,5 @@
> +  SyncScheduler.scheduleNextSync(requestedInterval);
> +  do_check_true(SyncScheduler.nextSync <= Date.now() + requestedInterval);
> +  do_check_eq(SyncScheduler.syncTimer.delay, requestedInterval);
> +
> +  // Request a sync at the smallest possible number.

interval.

@@ +254,5 @@
> +  run_next_test();
> +});
> +
> +add_test(function test_scheduleNextSync_backoff() {
> + _("scheduleNextSync() will honour backoff in all scheduling requests.");

Hmm, US or international english?
Comment 3 Richard Newman [:rnewman] 2011-10-04 20:11:40 PDT
Waiting for additional test fixes for altered logic.
Comment 4 Philipp von Weitershausen [:philikon] 2011-10-04 20:51:14 PDT
Created attachment 564749 [details] [diff] [review]
v2

Addressed nits and made two failing tests pass.
Comment 5 Richard Newman [:rnewman] 2011-10-04 20:53:09 PDT
Comment on attachment 564749 [details] [diff] [review]
v2

Review of attachment 564749 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/policies.js
@@ +349,5 @@
>    /**
>     * Set a timer for the next sync
>     */
>    scheduleNextSync: function scheduleNextSync(interval) {
> +    // If no interval was specific, use the current sync interval.

s/specific/specified.
Comment 6 Philipp von Weitershausen [:philikon] 2011-10-04 20:56:17 PDT
https://hg.mozilla.org/services/services-central/rev/2fc5fa5a744d
Comment 7 Philipp von Weitershausen [:philikon] 2011-10-05 11:20:52 PDT
STRs for QA:

All score and and back-from-idle triggered syncs should now obey backoff, too. They previously would ignore backoff and start a sync immediately.

* Score triggered syncs happen when bookmarks, passwords, and prefs are modified.
* Back-from-idle triggered syncs occur when the machine was idle for the duration of 'services.sync.scheduler.idleTime' (default is 300 seconds) and the user comes back to the machine.
Comment 8 Philipp von Weitershausen [:philikon] 2011-10-05 11:57:29 PDT
Note that score and back-from-idle triggered syncs only happen when there are at least 2 devices connected to the account. Single device users will not see this.
Comment 9 Tracy Walker [:tracy] 2011-10-05 12:30:28 PDT
score triggers and back-from-idle was ignored during back-off timer.

restart sync happened as expected, because the nextSync timer is not persisted across sessions.
Comment 10 Philipp von Weitershausen [:philikon] 2011-10-05 16:53:39 PDT
Comment on attachment 564749 [details] [diff] [review]
v2

Requesting approval for Aurora. This patch will help us further regulate load on the Sync infrastructure as it makes more sync triggers aware of the backoff setting. Patch is already verified by Services QA.
Comment 11 Philipp von Weitershausen [:philikon] 2011-10-05 16:54:50 PDT
Created attachment 565075 [details] [diff] [review]
patch for beta and release

Requesting approval for Beta and Release. Please see comment 10 for justification.
Comment 12 Philipp von Weitershausen [:philikon] 2011-10-05 17:08:06 PDT
https://hg.mozilla.org/mozilla-central/rev/2fc5fa5a744d
Comment 13 Philipp von Weitershausen [:philikon] 2011-10-06 16:46:48 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/ac11059899af
Comment 14 Philipp von Weitershausen [:philikon] 2011-10-06 16:57:54 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/8f6cce824102
Comment 15 Philipp von Weitershausen [:philikon] 2011-10-06 17:12:07 PDT
Thanks for the approvals, Christian and Asa. This bug depends on bug 691612 (was incorrectly marked as a blocker, sorry), so it won't make sense to land this without landing that first.
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 11:26:49 PDT
qa- as there does not appear to be much QA can do to verify this fix in Firefox.
Comment 17 Philipp von Weitershausen [:philikon] 2011-10-13 12:04:12 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #16)
> qa- as there does not appear to be much QA can do to verify this fix in
> Firefox.

That's not true. Services Ops can easily configure the staging servers to send backoff notices. Perhaps the folks from Services QA can chime in here and help out with verifying.

This fix is absolutely vital for the health of our Sync server infrastructure. We need to make sure it works in the released versions of Firefox.
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 12:07:40 PDT
Thanks for clarifying. Feel free to set qa+ in the future when a qa- can be proven wrong.
Comment 19 James Bonacci [:jbonacci] 2011-10-13 12:24:29 PDT
So, yes. Tracy and I can work with petef to get this verified from the server side.
Comment 20 Philipp von Weitershausen [:philikon] 2011-10-13 12:34:43 PDT
(In reply to James Bonacci [:jbonacci] from comment #19)
> So, yes. Tracy and I can work with petef to get this verified from the
> server side.

There is no server side to this in terms of code. petef can help you set the backoff header on stage, but the code that needs verifying is all client side. Tracy verified it for the s-c hand-off already, but we need to ensure it's fixed in Aurora and Beta as well.
Comment 21 Tracy Walker [:tracy] 2011-12-15 08:42:50 PST
this was verified in beta, when beta was 8.  Shortly, I'll be verifying on current beta, which is 9.  status-firefoxX: "verified" will be very useful.
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 14:36:36 PST
Tracy or James, can one of you confirm if this is fixed in Firefox 10? I see it's been verified on Firefox 8 and 9 already.
Comment 23 Tracy Walker [:tracy] 2011-12-28 15:19:32 PST
yes, it's good on 10
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 15:49:47 PST
Thanks Tracy.

Note You need to log in before you can comment on or make changes to this bug.