fix(sync): use remotePollInterval for the spaces refresh - #1047
Conversation
5dd5411 to
5df2a4f
Compare
If I remember correctly, this is so that an application writing a file does not immediately trigger an upload, but give the application(system) the chance to write more data before attempting to upload it. (think for example a big DOC file with a lot of images saving on a busy system) |
Yes the minimal file age is 2s and we gather change events for 10s in the folder watcher, before we trigger a sync. Overall, I'm not a fan of making those configurable as it will make reading logs or evaluating bug reports way harder, with minimal benefits. |
|
If a user switches between two devices we usually recommend to right-click on a space and force a sync. |
The SpacesManager refreshed the drives list every 30 seconds regardless of the remotePollInterval setting and the server's pollinterval capability, although the ETagWatcher schedules the syncs from that refresh. The same setting already governs the connection check in AccountState, so the two intervals were inconsistent. The refresh timer now takes its interval from ConfigFile::remotePollInterval(), i.e. the server capability if it is above 5 seconds, otherwise 30 seconds, overridable by remotePollInterval in the config file (milliseconds, minimum 5000). The effective interval is logged whenever it changes so it shows up in bug reports. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5df2a4f to
f224c00
Compare
|
Thanks for the context. I understand the support concern: two more knobs whose values are unknown when reading a log. A few numbers from a 1 GbE LAN, same server, same client, to put "minimal benefits" into perspective: upload of 10 x 10 MB 13.4 s -> 4.7 s, upload of 100 x 10 KB 15.4 s -> 8.4 s, download of 100 x 10 KB 23.7 s -> 3.4 s (worst case 30 s -> 5 s). With the defaults the client waits longer than it transfers. To make this less of a knob I have reduced the PR to the SpacesManager change only: the existing Would that be acceptable? |
|
To me yes but @TheOneRing needs to decide. Just for the record, not important but I think your PR changes then from 30 to 60? (Just guessing, I didn't actually run it) @dragotin What happened to the long polling / websocket discussion we had some time ago? |
|
While the 30s where just a magic number we are too close to a release in any way. I'll mark the pr as |
|
Thanks. On the 60: no, the interval stays at 30 s with this server. |
Problem
SpacesManagerrefreshes the drives list everyrefreshTimeoutC = 30s(src/libsync/graphapi/spacesmanager.cpp). TheETagWatcherschedules syncs from that refresh, so this is effectively the poll interval for remote changes. TheremotePollIntervalsetting and the server capabilitypollintervalhave no influence on it, although the same setting already governs the connection check inAccountState.Change
SpacesManager::refresh()now sets the refresh timer fromConfigFile::remotePollInterval(capabilities.remotePollInterval()): the server capability if it is above 5 seconds, otherwise the 30 second default, overridable byremotePollIntervalin the config file (milliseconds, minimum 5000). No new setting is introduced. The effective interval is logged whenever it changes (sync.graphapi.spacesmanager), so it is visible in bug reports.Behaviour without the setting and without a server capability is unchanged (30 s).
Result
Measured on a 1 GbE LAN against an OpenCloud 7.1 server with
remotePollInterval=5000: a remote change of 100 x 10 KB arrived after 3.4 s instead of 23.7 s (worst case 5 s instead of 30 s), 10 x 10 MB after 5.5 s instead of 6.1 s, 400 MB after 7.6 s instead of 13.6 s.An earlier version of this PR also made the folder watcher delay configurable; that part was dropped after review and will be raised separately as an issue about the default.
🤖 Generated with Claude Code