-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Updated RLQS Response handling to not reset TokenBucket state #36478
Updated RLQS Response handling to not reset TokenBucket state #36478
Conversation
… hasn't actually changed its config, otherwise token state will always reset Signed-off-by: Brian Surber <[email protected]>
… case's handling to be consistent across all actions Signed-off-by: Brian Surber <[email protected]>
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.
Thanks for working on this!
PTAL at CI build failure which is real.
@@ -77,6 +77,20 @@ void RateLimitClientImpl::sendUsageReport(absl::optional<size_t> bucket_id) { | |||
stream_->sendMessage(buildReport(bucket_id), /*end_stream=*/false); | |||
} | |||
|
|||
bool cacheHasTokenBucket(const ::envoy::type::v3::TokenBucket& token_bucket, |
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.
This function is not used. Remove?
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.
Cleaned up, thanks.
/wait |
Signed-off-by: Brian Surber <[email protected]>
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.
lgtm, thanks!
…nvoyproxy#36478) Commit Message: When an RLQS response comes in that would create a duplicate TokenBucket (because the underlying config hasn't changed), treat this as a refresh to the TTL but don't reset the TokenBucket's state by recreating it. Additional Description: Risk Level: Low Testing: Unit testing Docs Changes: Release Notes: Platform Specific Features: --------- Signed-off-by: Brian Surber <[email protected]> Signed-off-by: anitabyte <[email protected]>
…nvoyproxy#36478) Commit Message: When an RLQS response comes in that would create a duplicate TokenBucket (because the underlying config hasn't changed), treat this as a refresh to the TTL but don't reset the TokenBucket's state by recreating it. Additional Description: Risk Level: Low Testing: Unit testing Docs Changes: Release Notes: Platform Specific Features: --------- Signed-off-by: Brian Surber <[email protected]> Signed-off-by: Steven Jin Xuan <[email protected]>
Commit Message: When an RLQS response comes in that would create a duplicate TokenBucket (because the underlying config hasn't changed), treat this as a refresh to the TTL but don't reset the TokenBucket's state by recreating it.
Additional Description:
Risk Level: Low
Testing: Unit testing
Docs Changes:
Release Notes:
Platform Specific Features: