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

Add retry for rooftop sites collection #12 #26

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

autoSteve
Copy link
Collaborator

What I think is going on with #12 is that 'random' 429 errors from Solcast (i.e. "Back off, we're busy!") prevent the integration from loading properly on startup. This causes sensors to go missing, which is sub-optimal.

Caching rooftop sites is one option, and I like it, but that carries real-world use case implications (add array? delete array? modify array? rename array?). There would have to be an easy and intuitive way to force refresh the cache, which is unlikely. Or a once-a-day attempt at refreshing the cache.

Another option is simply retrying until the data can be had.

This marge attempts a retry approach, where for up to one minute it will try every six seconds.

It's going to be very difficult to test this, because how can you guarantee Solcast will be busy on any given hour/minute/week/etc. and return a 429? And the only real way of testing is repeatedly restarting HA so that the integration loads the rooftop sites, which it only does on startup.

@autoSteve autoSteve requested a review from BJReplay as a code owner June 13, 2024 07:44
Copy link
Owner

@BJReplay BJReplay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A well thought out approach. I agree - I don't see how this is easy to test (at least not without a MITM proxy).

@BJReplay BJReplay merged commit 3e469ee into v3 Jun 13, 2024
4 checks passed
@autoSteve autoSteve deleted the Retry-gather-rooftop-sites-data-for-up-to-1-minute branch June 13, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants