-
-
Notifications
You must be signed in to change notification settings - Fork 2
SlidingWindowCounter spec completed, some functionality copied over from token bucket #72
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
Conversation
| timestamp: number; | ||
| } | ||
|
|
||
| export interface RedisWindow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You created interface RedisWindow which looks the same as the interface RedisBucket. Any reason you can't use RedisBucket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or rename RedisBucket to a name that works for both buckets and windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going to add more values in the future to track the rolling and fixed window timestamps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case the new type should extend RedisBucket to keep things DRY.
At a higher level, what data does Redis need to track? My understanding is the complexity of all requests in the previous window along with the complexity of all requests in the current window. In each case the timestamp could just represent the beginning of the window. I don't think we need to track the timestamp of each request in Redis.
If we don't need anything else then for each user we just need to store two RedisBuckets either under to separate keys or both as an array. In the latter case then we would just need to type something as [RedisBucket, RedisBucket]
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline
evanmcneely
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved pending the resolution of my one comment about the new interface created.
shalarewicz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to discuss high-level approach before approving. See comments for more details
| timestamp: number; | ||
| } | ||
|
|
||
| export interface RedisWindow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case the new type should extend RedisBucket to keep things DRY.
At a higher level, what data does Redis need to track? My understanding is the complexity of all requests in the previous window along with the complexity of all requests in the current window. In each case the timestamp could just represent the beginning of the window. I don't think we need to track the timestamp of each request in Redis.
If we don't need anything else then for each user we just need to store two RedisBuckets either under to separate keys or both as an array. In the latter case then we would just need to type something as [RedisBucket, RedisBucket]
Thoughts?
|
updated |
Summary
Just like the title says, this is just a spec layout of the functionality to come.
Type of Change
Issues
closes #68