Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

default pollinterval 30000 msec #39143

Merged
merged 1 commit into from
Feb 25, 2022
Merged

default pollinterval 30000 msec #39143

merged 1 commit into from
Feb 25, 2022

Conversation

jnweiger
Copy link
Contributor

@jnweiger jnweiger commented Aug 27, 2021

The desktop client uses a default poll interval of 30000 msec whenever the capabilities announce something less than 5000.
To get in sync with this behaviour, I suggest to also change the value here to 30000.

A value of 60 also leads to confusion, as users assume the unit is seconds, which it is not.

Suggestion for future: Hardcode the unit in the name of the variable, like. e.g. pollinterval_msec

Related doc issue: https://github.com/owncloud/docs/issues/3970

Doc issue transferred to owncloud/docs-server#109

@jnweiger jnweiger changed the title default pollintervall 30000 msec default pollinterval 30000 msec Aug 27, 2021
@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiCapabilities-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31993/57/1

@phil-davis
Copy link
Contributor

phil-davis commented Aug 27, 2021

There are scenarios in apiCapabilities/capabilities.feature that have this value. Those need to be adjusted.

https://drone.owncloud.com/owncloud/core/31993/57/13

@phil-davis
Copy link
Contributor

pollinterval seems to have been added in PR #1584 a long time ago.
There is no mention there of if this is seconds or milliseconds, or about what a server might hope that a client will do with the value.

pollinterval does not seem to be used internally by the server anywhere. So I guess it is a "recommendation" to clients that care, to advise how often the server feels "comfortable" with being polled by clients?

And it looks like the default value can be overridden on a server by putting it in config/config.php but it is not documented in config/config.sample.php

@dragotin
Copy link
Contributor

This is the PR (new in 2.9) in the desktop client, that uses this: owncloud/client#8777

I was implementing it under the impression that nothing uses that capability so far. The idea is to give admins of large installations a chance to "adjust" the load from the desktop clients by expanding the remote poll interval a bit over 30 seconds, which was the default.

We decided to change the unit from seconds to Microseconds because that was the best way to decide if the admin really set that value or if the client reads an default value from the capabilities. The client only takes values > 5000 into account as a result.

I hope that clarifies a bit.

@butonic
Copy link
Member

butonic commented Feb 21, 2022

yes, please change the default to align with the 30sec / 30000msec used by clients

  • needs updated tests

@phil-davis phil-davis self-assigned this Feb 22, 2022
@owncloud owncloud deleted a comment from update-docs bot Feb 22, 2022
The desktop client uses a default poll interval of 30000 msec whenever the capabilities announce something less than 5000.
To get in sync with this behaviour, I suggest to also change the value here to 30000.

A value of 60 also leads to confusion, as users asume the unit is seconds, which it is not.

Suggestion for future: Hardcode the unit in the name of the variable, like. e.g. `pollinterval_msec`
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@mmattel
Copy link
Contributor

mmattel commented Feb 22, 2022

Note following comment in owncloud/client#8777

Note: To use 60 seconds, the capability needs to contain the string "60!" for the reason that 60 was the default in the capas, but using that would change the behaviour of all clients which needs to be avoided.

Is this still valid and does this has an impact to documentation?

@TheOneRing
Copy link
Contributor

Note following comment in owncloud/client#8777

Note: To use 60 seconds, the capability needs to contain the string "60!" for the reason that 60 was the default in the capas, but using that would change the behaviour of all clients which needs to be avoided.

Is this still valid and does this has an impact to documentation?

See #39143 (comment)

@phil-davis
Copy link
Contributor

I raised client PR owncloud/client#9465 to adjust some client comments etc.

IMO this core PR is ready to merge - the general agreement is that the unit-of-measure for pollinterval will be advertised/documented as milliseconds, and all clients that take any notice of it will implement/do implement it that way.

Can we merge?

@phil-davis
Copy link
Contributor

Merging - if anyone objects we can modify something, but IMO this is "a good thing".

@jnweiger
Copy link
Contributor Author

Confirmed fixed in 10.10.0 rc2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants