-
Notifications
You must be signed in to change notification settings - Fork 11
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
Github commit API rate limit #309
Comments
Is that from fetching the git commit history? |
Yes |
Should we cache the log into file(s) within our repo, so it doesn't need to hit the real API? |
Yeah might need to do that |
Or rmaybe site can just be modified as it seems to reload every time even if the only change is the selected commit, should only reload when doing a new suburb. |
Doesn't your new app-caching thing do that? |
Not in its current state as I don't know how long it can safely be cached for because it might get updated in a few hours all the way up to 17 days. |
I have some code that generates a changelog for each result file. Not sure it's a good idea: either adding 15,000 files to git, or putting them per-state (which are big). Plus we need to maintain the file, which naturally causes more commits. Maybe we can include "last run changes" at the start of next cycle... |
Technically, a client can know whether it needs to refresh the commit-log cache based on the state processed_date in results/combined-suburbs.json vs the date of the newest commit in the cache (for a particular suburb). |
I'll draft a PR that creates an index file (as small as possible), and uses it from the frontend. If that looks OK, we can add process to update the index, cache it from the client, etc. Will use short-hashes, and unixtime, and truncated filenames to minimise size. |
Added a call to update the index at the start of processing - this will add any commits that it's missing. Can't do it at the end (and include this-cycle commits) because we don't know the commit hash yet, and otherwise would need two commits (ugly). |
I can try updating the service worker to read the date of the latest commit and set it not to expire for 2 weeks from that date or if 2 weeks has already elapsed perhaps 1 day. |
Since the git-commit-log of all the suburbs are in the same file now, I think just caching it for a day would be fine (see the PR). #317 |
@LukePrior thoughts on PR? |
Just got this, should be error handled in frontend
{"message":"API rate limit exceeded for XXX. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"}
The text was updated successfully, but these errors were encountered: