443471 - Request: Configurable Sync Interval

Status: RESOLVED WONTFIX

(Cloud Services :: General, enhancement)

Description

[Dave P]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: 0.2

"Service.Main" begins the process "Running scheduled sync" every 60 seconds. This should be a user configurable interval to better accommodate those of us with under powered computers or limited bandwidth.

Ideally it should also allow the interval to be set to 0 which would result in the scheduled sync being disabled and Weave only syncing on demand.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.



If turning scheduled sync on and off (as well as changing the schedule interval) was easily accessible in Preferences, this would probably also fix Bug 436464 (Ability to have Weave pause syncing).

It would also mitigate, to some extent, the situation seen in Bug 443085 (Delay between sync tryes must contain random part to prevent rush on locks) but, ideally, the two could be combined to add a random number of seconds to the configurable interval.

Comment 2

[Dan Mills [:thunder]]

Thanks Miha, I hadn't noticed this bug before.

Comment 3

[Todd Cooper]

This would help with a lot of the problems I am seeing

Comment 4

[Todd Cooper]

Attachment: proposed patch

Adds sync tunable to weave... affects files:
chrome/content/preferences.xul
chrome/locale/en-US/preferences.dtd
defaults/preferences/sync.js
modules/service.js

Comment 5

[Todd Cooper]

Attachment: adding same sync patch uncompressed

Adds sync tunable to weave... affects files:
chrome/content/preferences.xul
chrome/locale/en-US/preferences.dtd
defaults/preferences/sync.js
modules/service.js

Comment 6

[Dan Mills [:thunder]]

Comment on attachment 333953 [details] [diff] [review]
adding same sync patch uncompressed

>+<!ENTITY syncInterval.description           "Sync interval in seconds">

Seconds sounds like the wrong unit for a user-facing feature.  I think minutes would be more appropriate.

> // How long we wait between sync checks.
>-const SCHEDULED_SYNC_INTERVAL = 60 * 1000 * 5; // five minutes
>+//const SCHEDULED_SYNC_INTERVAL = 60 * 1000 * 5; // five minutes

Remove if no longer needed.

>+  let local_syncInterval = Utils.prefs.getIntPref("syncInterval");
>+  this._log.config("Weave sync interval set to " + local_syncInterval);

nit: remove "local_", or call it localSyncInterval.
nit2: since we print this information when the timer is set up (below), we don't need to print it here, right?

>+    let local_syncInterval = Utils.prefs.getIntPref("syncInterval");
>+    this._scheduleTimer.initWithCallback(listener,   
>+					 local_syncInterval,
>+					 // was SCHEDULED_SYNC_INTERVAL,
>                                          this._scheduleTimer.TYPE_REPEATING_SLACK);
>+    this._log.config("Weave sync set to " + local_syncInterval);

Same style nits as above: remove commented-out code, use camelCase variable names.

Aside from the above, I have some additional concerns:

1) we should reset the timer when that pref changes.  See our observer implementation, it should be easy to add.

2) the sync interval is not the only value that governs how often data will get synced.  Tracker scores, thresholds, and the decrement step all play a part as well.  Thus, I think it would be misleading to have this pref ui as-is (but I think it would be too complicated to have the other values exposed).  Perhaps a drop-down with some simpler options might be a good alternative.  For example:

Attempt to sync items [As soon as possible        |v]
                      |As soon as possible          |
                      |Within a few hours           |
                      |Never, I will do it manually |
                      +-----------------------------+

Note that we can still have preferences for each value in about:config.

Clearing review? for now.

Comment 7

[Dan Mills [:thunder]]

Btw Todd, your patch is a great start, and I think a lot of users (specially 'power' users) want this feature.  So thanks for helping out!

If you are working on this bug, you can take it (assign it to yourself), that way others can know someone is working on it.

Cc'ing cbeard for his opinion on the prefs UI.

Comment 8

[Todd Cooper]

I do not have enough privs. to assign this to myself.

I am fixing the nits and changing the UI to read minutes.
I am looking into the other changes.

Comment 9

[Todd Cooper]

Attachment: add sync patch with nits fixed and UI in minutes

Comment 10

[Todd Cooper]

I don't see how the observer implementation works.
Is there a place that the code runs through when an item in the UI changes?

Also I think I like the idea of syncing with a rougher idea.
We can even tune the other changes to scale (namely the 75 threshold)

I would propose
Attempt to sync items [Every hour                 |v]
                      |As soon as possible          |
                      |Every hour                   |
                      |Every few hours              |
                      |Every day                    |
                      |Never, I will do it manually |
                      +-----------------------------+

I would make the default every hour and warn users that syncing more often
could cause performance problems.

Also, how can I change the assigned to to me?

Comment 11

[Dan Mills [:thunder]]

Oops, sorry for not reviewing this before.  You can assign the bug to yourself (if you're still interested) by going to the top of the bug where it says "Assigned To" and clicking the (edit) link there.

Setting for a future version, though if you add the drop-down UI as above I would review and land for 0.3.

Comment 12

[Dan Mills [:thunder]]

Comment on attachment 334325 [details] [diff] [review]
add sync patch with nits fixed and UI in minutes

This may have bitrotted a bit, since service.js has changed a bunch.

The patch is looking pretty good, but still has the sync interval UI in minutes, rather than the drop-down.

Comment 14

[Mike Connor [:mconnor]]

We've moved to a model where we sync on both timers and score thresholds, so I think this is WONTFIX at this point, since the UI would be not-really deterministic, except as a max duration, and I don't think we really want to let people set this to DDoS levels in the UI anyway... :)