Using DataCoordinator instead of direct update #134087
Conversation
home-assistant
bot
added
bugfix
cla-signed
integration: vesync
Quality Scale: No score
labels
|
Hey there @markperdue, @webdjoe, @TheGardenMonkey, @cdnninja, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| _LOGGER, | ||
| name=DOMAIN, | ||
| update_method=async_update_data, | ||
| update_interval=timedelta(seconds=30), |
There was a problem hiding this comment.
Could we set this to 60? At 30 it would max the count at 3 devices. 60 will solve that.
There was a problem hiding this comment.
It should not max out, there should be only one request regardless of the number of devices per second which would be (25 * 3600 / 30) = 2880 per day, right?
The error message mentioned "3200 + 1500 * user owned device number" which was a issue if there were lots of total entities.
There was a problem hiding this comment.
Ahh let me check the underlying code. I assumed that Vesync library was just doing a for each loop. The endpoints are different so I believe it is but I'll double check.
There was a problem hiding this comment.
This update on the vesync manager updates all the devices within a 30 sec interval check.
There was a problem hiding this comment.
There will be another 2880 calls per day due to pulling the device list in each manager.update() call, which goes beyond the limit. It would be better to use manager.update_all_devices() to avoid the unnecessary pulling of the device list. Adding / removing devices can be done manually by a service
I am going to remove the half baked rate limiting logic, that was done at the very beginning of the project and probably the 5th line of python code I ever wrote. My opinion is that the rate limiting should be done at the user level, not the library level. Happy to hear if anyone has another opinion.
There was a problem hiding this comment.
I can't believe I never caught that.
@joostlek can I make a release that can be bumped in this PR?
There was a problem hiding this comment.
2.1.15 is published with the fix
There was a problem hiding this comment.
Thanks for the prompt fix. I rebase my branch and bumped the pevsync version and also selected the Dependency upgrade checkbox in the PR.
There was a problem hiding this comment.
Could we include the bump of the lib in a preliminary PR so it's not included in this (quite large already) PR
There was a problem hiding this comment.
Sure. I have submitted #134156 for consuming 2.1.15. I will adjust this PR once that has been merged.
Using DataCoordinator allows us to reduce number of web calls being made. Some devices can have multiple entities which increases the number of web calls and hitting the daily limit. DataCoordinator reduces that to just 1 call.
iprak
force-pushed
the
use-data-coordinator-2
branch
from
81995c5 to
20d01a3
Compare
This avoids fetching device list every time.
Proposed change
This request switches Vesync devices to use DataCoordinator pattern instead of each entity updating itself. The latter can run over the daily webcalls limits where the web end point returns an error message like
Current user's daily request quota has been used up. The quota formula is \"3200 + 1500 * user owned device number\". Please note that we may change the constant in the formula.Type of change
Additional information
This should partially address #131781 which in my case was due to daily quota being used up.
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: