Fix retry when Met config entry fails #70012
Conversation
probot-home-assistant
bot
added
bugfix
integration: met
small-pr
|
Hey there @Danielhiversen, @thimic, mind taking a look at this pull request as it has been labeled with an integration ( |
| # From https://api.met.no/weatherapi/locationforecast/2.0/documentation | ||
| URL = "https://api.met.no/weatherapi/locationforecast/2.0/complete" |
There was a problem hiding this comment.
We cannot change this. As we are a heavy traffic consumer, we have been assigned a different end-point.
(I think...)
There was a problem hiding this comment.
We cannot change this. As we are a heavy traffic consumer, we have been assigned a different end-point.
Gotha, but current URL here timeout 99% of the time :-S
There was a problem hiding this comment.
Sorry for tagging you @geira, could you maybe provide insight on that?
There was a problem hiding this comment.
The reason Home-Assistant has its own custom subdomain is that the traffic is way above the TOS limit of 20 requests/second, and is currently using about 20% of the total api traffic. For this reason we need to be able to monitor and route traffic to optimize resources. As recently mentioned on our mailing list we are experiencing network problems with slow downloads on the main gateway. Since we have an Easter holiday systems freeze we decided to route the most trafficed subdomains to our standby gateway which previously had been running idle, until we can set up a new redundant pair of servers to balance the load. Please do not use momentary statistics taken during a notified emergency as an excuse to circumvent the TOS.
There was a problem hiding this comment.
Right, so it is a service disruption, thanks for that valuable information that is appreciated.
Please do not use momentary statistics taken during a notified emergency as an excuse to circumvent the TOS.
Nobody said we would, the review is preventing it even and we reach out to understand why the assigned endpoint isn't functioning.
Please don't approach things from a negative context and throw around TOS policies and such. We are just asking and reaching out.
Have a great Easter weekend @geira 🥚 🐰
👍
There was a problem hiding this comment.
Added a note for the future.
There was a problem hiding this comment.
That is helpful, thanks @chemelli74 👍
| await self._weather_data.fetching_data() | ||
| resp = await self._weather_data.fetching_data() | ||
| if not resp: | ||
| raise asyncio.TimeoutError |
There was a problem hiding this comment.
Hmm, this is an asyncio error, which is kinda awkward to raise here? As in, that is not managed by us.
Maybe raise a custom error (from HomeAssistantError) or UpdateFailed instead?
There was a problem hiding this comment.
As far as I understand this update function is used only here:
core/homeassistant/components/met/__init__.py
Line 109 in 97a04dc
I think it would be better to move this fetching part directly there and raise raise UpdateFailed instead of raising here and raising again.
Another option is to change the return type to allow None
Proposed change
Handle when upstream
fetching_data()fails: need to raise an exception as the method return only true/false.Meanwhile, also the api URL is updated based on the official documentation (old one gives timeout 99% of the time)Type of change
Additional information
Checklist
black --fast 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..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: