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

Request fewer GitHub releases and cache string URLs in Netkan #2950

Merged
merged 1 commit into from
Dec 24, 2019

Conversation

HebaruSan
Copy link
Member

Problem

New inflation error for HalfRSS-Textures2048: GitHub API rate limit exceeded.
New inflation error for HalfRSS-Textures4096: GitHub API rate limit exceeded.
New inflation error for HalfRSS-Textures8192: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-Afterburner: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-AutoBalancingLandingLeg: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-DisableTempGauges: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-DailyFunds: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-DestroyAll: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-EditorCamUtilities: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-FillSpotsWithTourists: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-FPSLimiter: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-FPSViewer: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-KerbalScienceExchange: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-ModifiedExplosionPotential: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-PhysicalTimeRatioViewer: GitHub API rate limit exceeded.
New inflation error for KerboKatzSmallUtilities-RecoverAll: GitHub API rate limit exceeded.
New inflation error for KerboKatzUtilities: GitHub API rate limit exceeded.

Cause

The exact reason this happens is uncertain, but we suspect we're triggering GitHub's abuse prevention limits:

  • We're probably requesting too many GitHub releases at once (100)
  • We're probably requesting the same API URLs too many times in a short period

Changes

  • Now we request 10 GitHub releases instead of 100, and if we need more (rare!), we request 10 more, and so on. This will cut down on the server load to handle our GitHub API requests.
  • Now CachingHttpService.DownloadText caches its results for 3 minutes, so any repeated requests for the same URL (e.g., if we inflate all of the KerboKatz modules in rapid succession) will only hit the server once. This will avoid the possibly abuse-y behavior of hitting the same URL over and over, and also probably speed up the Inflator a little bit (since memory is a lot faster than network).

Fixes #2949 (hopefully!).

@HebaruSan HebaruSan added Bug Pull request Netkan Issues affecting the netkan data Network Issues affecting internet connections of CKAN labels Dec 24, 2019
@HebaruSan
Copy link
Member Author

I want to see whether this helps, but my reviewers are busy for holiday break. 🎄
Merging and will keep an eye and revert if the bot stops working.

@HebaruSan HebaruSan merged commit 5ad7d62 into KSP-CKAN:master Dec 24, 2019
@HebaruSan HebaruSan deleted the fix/nk-cache-text branch December 24, 2019 01:08
@DasSkelett
Copy link
Member

Hehe. I did take a short look over the code just now, but without testing or anything.
It looks good, maybe it could've used some debug log statements to see whether the cache has been hit or not, but function-wise I think it's good.

@HebaruSan
Copy link
Member Author

Nailed it!

image

So just like in #2928, if we get rate limited when the limit is not exceeded, it's because of repeated calls to the same URL. Which should be impossible now.

@HebaruSan
Copy link
Member Author

Hmm, I think this froze the Inflator after about 2 days, maybe we used too much memory. We can purge more pro-actively, but that has to be done carefully to avoid taking up a ton of CPU.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Netkan Issues affecting the netkan data Network Issues affecting internet connections of CKAN Pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] NetKAN being rate limited by authenticated requests
2 participants