Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make RateLimiter not Customizable #10378

Closed
wants to merge 1 commit into from
Closed

Make RateLimiter not Customizable #10378

wants to merge 1 commit into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Jul 17, 2022 •
edited
Loading

Differential Revision: D37914865

(PR created for informational/testing purposes only.)

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D37914865

@hx235 hx235 self-requested a review July 17, 2022 23:19
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D37914865

@ajkr @facebook-github-bot
Make RateLimiter not Customizable (facebook#10378)
be076a3 
Summary:
(PR created for informational/testing purposes only.)

- Fixes lost dynamic updates to GenericRateLimiter bandwidth using `SetBytesPerSecond()`
- Benefit over facebook#10374 is eliminating race conditions with Configurable framework.

Pull Request resolved: facebook#10378

Differential Revision: D37914865

fbshipit-source-id: e2b455eb30058f9eea5fb2af870eaa7c77da6969
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D37914865

@ajkr ajkr mentioned this pull request Jul 18, 2022
Closed
siying pushed a commit that referenced this pull request Jul 18, 2022
@ajkr @siying
Make RateLimiter not Customizable (#10378)
a58fae9 
Summary:
(PR created for informational/testing purposes only.)

- Fixes lost dynamic updates to GenericRateLimiter bandwidth using `SetBytesPerSecond()`
- Benefit over #10374 is eliminating race conditions with Configurable framework.

Pull Request resolved: #10378

Reviewed By: pdillinger

Differential Revision: D37914865

fbshipit-source-id: d4f566d60ec9726d26932388c61671adf0ee0f30
@mrambacher
Copy link
Contributor

This is a blunt way to fix a bug in the GenericRateLimiter. This change prevents anyone from writing their own pluggable rate limiter.

It also prevents creating one from the OPTIONS file. This change is likely to break version compatibilty.

Most of this is unnecessary and can be solved in other means that would still allow a user or company to create pluggable Rate Limiters

@riversand963
Copy link
Contributor

It also prevents creating one from the OPTIONS file

One thought regarding this: if multiple databases share the same rate limiter instance, and application calls RateLimiter::SetBytesPerSecond() on the rate limiter object, the change will not be updated immediately in the OPTIONS files of the databases. Using the OPTIONS file to create objects that can be shared by multiple databases requires additional caution.

ajkr added a commit that referenced this pull request Jul 19, 2022
@ajkr
Make RateLimiter not Customizable (#10378)
66f369f 
Summary:
(PR created for informational/testing purposes only.)

- Fixes lost dynamic updates to GenericRateLimiter bandwidth using `SetBytesPerSecond()`
- Benefit over #10374 is eliminating race conditions with Configurable framework.

Pull Request resolved: #10378

Reviewed By: pdillinger

Differential Revision: D37914865

fbshipit-source-id: d4f566d60ec9726d26932388c61671adf0ee0f30
facebook-github-bot pushed a commit that referenced this pull request Jul 19, 2022
@ajkr @facebook-github-bot
Fix race conditions in GenericRateLimiter (#10374)
e576f2a 
Summary:
Made locking strict for all accesses of `GenericRateLimiter` internal state.

`SetBytesPerSecond()` was the main problem since it had no locking, while the two updates it makes need to be done as one atomic operation.

The test case, "ConfigOptionsTest.ConfiguringOptionsDoesNotRevertRateLimiterBandwidth", is for the issue fixed in #10378, but I forgot to include the test there.

Pull Request resolved: #10374

Reviewed By: pdillinger

Differential Revision: D37906367

Pulled By: ajkr

fbshipit-source-id: ccde620d2a7f96d1401bdafd2bdb685cbefbafa5
@pdillinger pdillinger mentioned this pull request Jul 28, 2022
Open
@mrambacher mrambacher mentioned this pull request Jul 29, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hx235 hx235 Awaiting requested review from hx235

Assignees
No one assigned
Labels
CLA Signed fb-exported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ajkr @facebook-github-bot @mrambacher @riversand963