Generate rate limit frontend api handler #5636
Conversation
Shaddoll
requested review from
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Pull Request Test Coverage Report for Build 018d6b8b-50b8-4c5f-95e5-5177c23645c8
💛 - Coveralls |
Shaddoll
force-pushed
the
ratelimit
branch
3 times, most recently
from
28aa13e to
50d3d50
Compare
Shaddoll
force-pushed
the
ratelimit
branch
2 times, most recently
from
37fa6eb to
091050e
Compare
There was a problem hiding this comment.
gonna stick myself in here for "I think this needs a careful read before approval, but broadly I like it". I just don't have the brain energy to do that right now.
main concern is that this makes a significant change to when the call occurs, and that needs to be verified to be safe.
whether we return a 429 or "bad request" error on floods of bad requests: eh, I think both are defensible, changing it seems fine to me. it's just worth checking if there are any particularly valuable ones that will be lost (I sincerely doubt it).
| func (h *apiHandler) allowDomain(requestType ratelimitType, domain string) bool { | ||
| switch requestType { | ||
| case ratelimitTypeUser: | ||
| return h.userRateLimiter.Allow(quotas.Info{Domain: domain}) | ||
| case ratelimitTypeWorker: | ||
| return h.workerRateLimiter.Allow(quotas.Info{Domain: domain}) | ||
| case ratelimitTypeVisibility: | ||
| return h.visibilityRateLimiter.Allow(quotas.Info{Domain: domain}) | ||
| default: | ||
| panic("coding error, unrecognized request ratelimit type value") | ||
| } | ||
| } |
There was a problem hiding this comment.
this is a fairly minimal change and if you prefer that then 👍 but:
we could take this opportunity to remove this method and the iota and directly call the appropriate limiter.
I didn't do that initially when adding these, to keep things minimal, but it's definitely due for a cleanup at some point. Does not have to be now though.
Shaddoll
force-pushed
the
ratelimit
branch
2 times, most recently
from
f95adb7 to
6662f3c
Compare
| if rp1 == nil { | ||
| err = validate.ErrRequestNotSet | ||
| return | ||
| } | ||
| if rp1.TaskToken == nil { | ||
| err = validate.ErrTaskTokenNotSet | ||
| return | ||
| } |
There was a problem hiding this comment.
at some point we should probably make some validation-wrappers that occur early and get rid of this need at random levels...
but 👍 yep, same as existing behavior, LGTM. I've been trying to come up with a nice way to build that kind of validator, but everything feels like a huge pain / very disconnected from where the checks matter...
What changed?
Generate rate limit frontend api handler code
Some fix:
Why?
To reduce manual work
How did you test it?
Potential risks
Release notes
Documentation Changes