Skip to content

ratelimit: per descriptor hits addend support and prefer uint64#802

Merged
psbrar99 merged 5 commits intoenvoyproxy:mainfrom
wbpcode:dev-per-descriptor-hits-addend
Dec 27, 2024
Merged

ratelimit: per descriptor hits addend support and prefer uint64#802
psbrar99 merged 5 commits intoenvoyproxy:mainfrom
wbpcode:dev-per-descriptor-hits-addend

Conversation

@wbpcode
Copy link
Copy Markdown
Member

@wbpcode wbpcode commented Dec 23, 2024

No description provided.

Signed-off-by: wangbaiping/wbpcode <wangbaiping@bytedance.com>
Copy link
Copy Markdown
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

high level question: if the number zero is given, it should work like "check the budget is not negative, and if not requests are allowed"? I am asking because if this would obviate the issue we had in #729

Signed-off-by: wangbaiping/wbpcode <wangbaiping@bytedance.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Dec 23, 2024

high level question: if the number zero is given, it should work like "check the budget is not negative, and if not requests are allowed"? I am asking because if this would obviate the issue we had in #729

The counter in redis is a number that counts all used tokens. So, we only need to check if the counter is larger than the threshold or not.

@mathetake
Copy link
Copy Markdown
Member

cool, so in other words, sending hits_addend=0 can be used to reject requests if the counter in redis is negative or zero ya?

Signed-off-by: wangbaiping/wbpcode <wangbaiping@bytedance.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Dec 23, 2024

cool, so in other words, sending hits_addend=0 can be used to reject requests if the counter in redis is negative or zero ya?

sorry, I may didn't make it clear. The counter will never be negative in the redis. It is a counter the will increases forever in the time window.

But the conclusion is right. hits_addend zero could also could be used to reject a request.

@mathetake
Copy link
Copy Markdown
Member

great, thank you for clarifying!

Signed-off-by: wangbaiping/wbpcode <wangbaiping@bytedance.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Dec 23, 2024

cc @psbrar99

@mathetake
Copy link
Copy Markdown
Member

also @zirain if you have any comments

}

func Max(a uint32, b uint32) uint32 {
func Max(a uint64, b uint64) uint64 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not a must, but is it find to remove this as golang support generic?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am not very familiar to go (I use c++ at most times orz), so not sure if a new generic Max is provided by the golang stdlib or not. If it is provided, it would be great to remove this function.

Copy link
Copy Markdown
Member

@zirain zirain Dec 23, 2024

Choose a reason for hiding this comment

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

// The max built-in function returns the largest value of a fixed number of
// arguments of [cmp.Ordered] types. There must be at least one argument.
// If T is a floating-point type and any of the arguments are NaNs,
// max will return NaN.
func max[T cmp.Ordered](x T, y ...T) T

looks like there's.

Copy link
Copy Markdown
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

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

LGTM, only a small nit.

Signed-off-by: wangbaiping/wbpcode <wangbaiping@bytedance.com>
@psbrar99
Copy link
Copy Markdown
Contributor

LGTM, thanks @wbpcode .

@psbrar99 psbrar99 self-requested a review December 27, 2024 01:03
@psbrar99 psbrar99 merged commit 38500fe into envoyproxy:main Dec 27, 2024
@mathetake
Copy link
Copy Markdown
Member

fantastic, thank you for your hard work! @wbpcode 👯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants