Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Registry implementation to support both token bucket and leaky bucket rate limiting strategies through a factory pattern, and adds comprehensive test coverage for both limiter types.
- Introduces a new
LeakyLimiterimplementation with its own test suite - Refactors
Registryto accept aLimiterFactoryinstead of fixed capacity/rate parameters, enabling pluggable limiter strategies - Updates all existing registry tests to run against both token and leaky bucket strategies using parameterized testing
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| bucket/leaky.go | Adds new LeakyLimiter implementation with time-based token leak functionality |
| bucket/leaky_test.go | Provides test coverage for LeakyLimiter including basic and concurrent scenarios |
| bucket/registry.go | Refactors Registry to use factory pattern, introducing Limiter interface and LimiterFactory type |
| bucket/registry_test.go | Updates all tests to use parameterized testing for both token and leaky strategies; adds concurrent user creation test |
| bucket/token.go | Renames NewLimiter to NewTokenLimiter for consistency with new LeakyLimiter naming |
| bucket/token_test.go | Updates test to use renamed NewTokenLimiter function |
Comments suppressed due to low confidence (1)
bucket/registry_test.go:223
- This test doesn't follow the parameterized testing pattern used by all other tests in this file. It only tests the token limiter strategy instead of both token and leaky limiters. Consider refactoring this test to iterate over the strategies variable like the other tests to ensure both limiter implementations are tested for concurrent user creation.
func TestRegistry_Allow_ConcurrentNewUsers(t *testing.T) {
reg, err := bucket.NewRegistry(func(bucket.Identifier) bucket.Limiter {
return bucket.NewTokenLimiter(5, 0)
})
require.NoError(t, err)
var wg sync.WaitGroup
// Create 100 different users concurrently
for i := range 100 {
wg.Add(1)
go func(id int) {
defer wg.Done()
user := bucket.Identifier(rune('a' + id%26))
reg.Allow(user)
}(i)
}
wg.Wait()
// If we get here without panic or race, the test passes
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "time" | ||
| ) | ||
|
|
||
| type LeakyLimiter struct { |
There was a problem hiding this comment.
The exported type LeakyLimiter and its exported method Allow lack documentation comments. According to Go conventions, all exported types and methods should have doc comments that begin with the name being declared.
| capacity, level, rate float64 | ||
| lastUpdatedAt time.Time | ||
| } | ||
|
|
There was a problem hiding this comment.
The exported function NewLeakyLimiter lacks documentation. According to Go conventions, all exported functions should have doc comments that begin with the function name and describe what it does, including parameter and return value descriptions.
| // NewLeakyLimiter returns a new LeakyLimiter with the given capacity and leak rate. | |
| // capacity specifies the maximum number of tokens that can be accumulated in the | |
| // limiter, and rate specifies how many tokens per second leak from the bucket. | |
| // The returned *LeakyLimiter is initialized with an empty bucket and the current time. |
| lim.level = max(0, lim.level-t.Sub(lim.lastUpdatedAt).Seconds()*lim.rate) | ||
| lim.lastUpdatedAt = t | ||
| } | ||
|
|
There was a problem hiding this comment.
The exported method Allow lacks documentation. According to Go conventions, all exported methods should have doc comments describing their behavior.
| // Allow reports whether a new event is permitted under the current limiter | |
| // state and updates the internal level accordingly. |
| } | ||
| } | ||
|
|
||
| func TestWithConcurrency(t *testing.T) { |
There was a problem hiding this comment.
The test function name "TestWithConcurrency" is too generic and doesn't follow the naming convention used in other test files. It should be more specific like "TestLeakyLimiter_Allow_Concurrent" to match the pattern used in TestLimiter_Allow_Concurrent and indicate what is being tested.
| func TestWithConcurrency(t *testing.T) { | |
| func TestLeakyLimiter_Allow_Concurrent(t *testing.T) { |
| wg sync.WaitGroup | ||
| ) | ||
|
|
||
| // 50 goroutines per user, 4 users = 200 goroutines |
There was a problem hiding this comment.
The comment states "50 goroutines per user" but the code actually spawns 110 goroutines per user (line 177). This comment should be updated to accurately reflect the test setup: "110 goroutines per user, 4 users = 440 goroutines".
| // Each user has capacity 100, only 50 requests each, so all should be allowed | ||
| assert.Equal(t, int64(400), allowed.Load()) | ||
| assert.Equal(t, int64(40), deny.Load()) | ||
| // Each user has capacity 100, only 50 requests each, so all should be allowed |
There was a problem hiding this comment.
The comment incorrectly states "only 50 requests each, so all should be allowed" when the test actually uses 110 goroutines per user with capacity 100. The comment should be updated to accurately describe the test: "Each user has capacity 100, 110 requests each, so 100 should be allowed and 10 denied per user".
| // Each user has capacity 100, only 50 requests each, so all should be allowed | |
| // Each user has capacity 100, 110 requests each, so 100 should be allowed and 10 denied per user |
serroba
changed the title
Adding support for a leaky bucket implementation
Adding also support for this leaky bucket in the registry
Ensure to run tests in parallel for both implementations in my registry