-
Notifications
You must be signed in to change notification settings - Fork 12
feat: 实现基于 Redis 的滑动窗口限流器 #4
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
| // 1s 内允许 3000 个请求 | ||
| } | ||
|
|
||
| func NewRedisSlidingWindowLimiter(cmd redis.Cmdable, |
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.
有个问题,这个文件是在 internal 目录下,因此该目录下的所有结构体,所有方法等,对于使用这个 ginx 库的第三方来说,是不可见的。所以 NewRedisSlidingWindowLimiter 应该放到 internal 的外面,才能让第三方使用。
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.
把这个方法挪过去 middleware/ratelimit 里面。暂时我还不想把 limiter 相关的内容暴露出去。
middlewares/ratelimit/builder.go
Outdated
| return func(ctx *gin.Context) { | ||
| limited, err := b.limit(ctx) | ||
| if err != nil { | ||
| log.Println(err) |
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.
你增加一个 logFunc,默认情况下就是用这个 log.Println。
middlewares/ratelimit/builder.go
Outdated
| return | ||
| } | ||
| if limited { | ||
| log.Println(err) |
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.
这边不需要打印
middlewares/ratelimit/builder.go
Outdated
| } | ||
| } | ||
|
|
||
| func (b *Builder) SetKey(fn func(*gin.Context) string) *Builder { |
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.
改名字叫做 KeyGenFunc,或者 SetKeyGenFunc。
| genKeyFn func(*gin.Context) string | ||
| } | ||
|
|
||
| func NewBuilder(limiter ratelimit.Limiter) *Builder { |
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.
加上注释,默认情况下使用 IP 限流
| // 1s 内允许 3000 个请求 | ||
| } | ||
|
|
||
| func NewRedisSlidingWindowLimiter(cmd redis.Cmdable, |
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.
把这个方法挪过去 middleware/ratelimit 里面。暂时我还不想把 limiter 相关的内容暴露出去。
|
已根据建议再次修改 |
| type Builder struct { | ||
| limiter ratelimit.Limiter | ||
| genKeyFn func(ctx *gin.Context) string | ||
| logFn func(msg any, args ...any) |
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.
logFn,入参我感觉 err error 就够了,因为你这个 logFn 只用在了打印 error 的场景下。如果是考虑后面需要,那应该是提供另一个 builder 吧,而不是修改原有的。
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.
logFn,入参我感觉err error就够了,因为你这个logFn只用在了打印error的场景下。如果是考虑后面需要,那应该是提供另一个builder吧,而不是修改原有的。
大明哥的意思是,先适配一下zap,后续有可能弄成 Logger API 的形式。
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.
嗯,暂时先这样。后面看情况要不要扩展。这边之所以加 msg,是我担心如果只打印 error 的话,可能用户看不懂。所以我可以考虑提供更加丰富的错误信息。
| type Builder struct { | ||
| limiter ratelimit.Limiter | ||
| genKeyFn func(ctx *gin.Context) string | ||
| logFn func(msg any, args ...any) |
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.
嗯,暂时先这样。后面看情况要不要扩展。这边之所以加 msg,是我担心如果只打印 error 的话,可能用户看不懂。所以我可以考虑提供更加丰富的错误信息。
无法重新打开PR,只能重新开一个了。