Refactor delete requests to use rate limiter - #1
Open
sebtoombs wants to merge 2 commits into
Open
Conversation
apkelly
reviewed
Sep 13, 2022
| } | ||
| } | ||
| // Only delete releases after the number to keep | ||
| const releasesToPurge = appReleases.slice(0, -toKeep); |
Contributor
There was a problem hiding this comment.
Does this slice keep the head of the array or the tail? We want to keep the head (as these are the latest versions).
Author
There was a problem hiding this comment.
Yep, this will remove the last toKeep items from the array - which I have just realised is incorrect. Pushed a change 🙏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Rate limit delete requests (why) to avoid getting rate limit banned
How
Using
rate-limiter-flexiblewith an in memory queue to limit requests. This allows us to tell Node to fire all our requests in parallel and then rely on the queue to handle meeting the rate limit requirement.What this means is we can fire off as many requests as possible as quickly as possible (possibly taking advantage of multiple threads) until we hit the rate limit and backoff.
The rate limiter config (index.js lines 9 - 16) needs to be set, or come from come config maybe? Currently (as an example) it is set as max. 1 request (points) per 2 seconds (duration). And requests beyond that will get added to the queue (maxQueueSize 100). If the queue overflows, these requests will return an error, same as if the request had failed (but message something like "Queue is full"). There's probably no limit on the queue size (beyond the max integer in Node & memory on the runner), so could probably up that to some ridiculous value.
Note
npm installand then commit that.