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 redis-based quota.Manager #1977

Merged
merged 8 commits into from
Nov 23, 2019

Conversation

adunham-stripe
Copy link
Contributor

What

This pull request adds two new packages:

  1. A Redis-based token bucket algorithm (under quota/redis/redistb). The underlying Lua script is based on what Stripe uses for our internal rate-limiters, is atomic, can handles tens of thousands of requests per second with no trouble, and is easily scaled via Redis Cluster.
  2. A Redis-based quota.Manager implementation that uses it (under quota/redis/redisqm). This package is currently fairly basic; all configuration is performed via code, not stored in Redis or another datastore. Note that it doesn't acquire multiple quota.Specs transactionally, but rather in order. This should be easy to extend in the future, if necessary.

Checklist

Copy link
Member

@AlCutter AlCutter left a comment

Choose a reason for hiding this comment

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

Thanks Andrew, this is super cool!
A few minor nits inline.

quota/redis/redisqm/manager.go Show resolved Hide resolved
quota/redis/redisqm/manager.go Outdated Show resolved Hide resolved
quota/redis/redisqm/manager.go Outdated Show resolved Hide resolved
quota/redis/redistb/embed_redis.go Show resolved Hide resolved
quota/redis/redistb/redistb_test.go Show resolved Hide resolved
@AlCutter
Copy link
Member

AlCutter commented Nov 23, 2019

Ah, there are a couple of other bits the presubmit picked up, too:

License check:

./quota/redis/redistb/embed_redis.go:10:license header not found
./quota/redis/redistb/gen.go:10:license header not found
./quota/redis/redistb/redistb_test.go:10:license header not found
./quota/redis/redistb/redistb.go:10:license header not found

Travis integration:
Looks like we need to install and spin up a local redis daemon, maybe?

--- FAIL: TestRedisTokenBucketIntegration (0.00s)
    redistb_test.go:65: failed to load script: dial tcp 127.0.0.1:6379: connect: connection refused

@AlCutter
Copy link
Member

AlCutter commented Nov 23, 2019

Travis integration:
Looks like we need to install and spin up a local redis daemon, maybe?

--- FAIL: TestRedisTokenBucketIntegration (0.00s)
    redistb_test.go:65: failed to load script: dial tcp 127.0.0.1:6379: connect: connection refused

Seems like it's easy enough to get a redis server by editing the services section in our .travis.yml file: https://docs.travis-ci.com/user/database-setup/#redis

@codecov
Copy link

codecov bot commented Nov 23, 2019

Codecov Report

Merging #1977 into master will increase coverage by 0.08%.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1977      +/-   ##
==========================================
+ Coverage   58.15%   58.23%   +0.08%     
==========================================
  Files         114      115       +1     
  Lines        9678     9750      +72     
==========================================
+ Hits         5628     5678      +50     
- Misses       3553     3572      +19     
- Partials      497      500       +3
Impacted Files Coverage Δ
quota/redis/redistb/redistb.go 72.22% <72.22%> (ø)
log/operation_manager.go 87.9% <0%> (-0.94%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6efc69...b860d9f. Read the comment docs.

@adunham-stripe
Copy link
Contributor Author

@AlCutter - Thanks for the feedback! Pushed changes, and it looks like tests are all passing now, including those using the Redis server 🎉

@AlCutter AlCutter merged commit 8974584 into google:master Nov 23, 2019
@AlCutter
Copy link
Member

@adunham-stripe Many thanks! Looking forward to seeing what else you might come up with :)

@adunham-stripe adunham-stripe deleted the adunham/redis-quota branch November 23, 2019 02:13
@adunham-stripe
Copy link
Contributor Author

@AlCutter - And thank you for the quick review!

// See the License for the specific language governing permissions and
// limitations under the License.

package redistb
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You're missing a package comment here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants