Skip to content

Make RateLimiter not Customizable#10378

Closed
ajkr wants to merge 1 commit intofacebook:mainfrom
ajkr:export-D37914865
Closed

Make RateLimiter not Customizable#10378
ajkr wants to merge 1 commit intofacebook:mainfrom
ajkr:export-D37914865

Conversation

@ajkr
Copy link
Copy Markdown
Contributor

@ajkr ajkr commented Jul 17, 2022 •
edited
Loading

Differential Revision: D37914865

(PR created for informational/testing purposes only.)

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jul 17, 2022

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
Copy Markdown
Contributor

facebook-github-bot commented Jul 18, 2022

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

@ajkr ajkr force-pushed the export-D37914865 branch from aad95fa to 260651f Compare July 18, 2022 01:42
@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
@ajkr ajkr force-pushed the export-D37914865 branch from 260651f to be076a3 Compare July 18, 2022 02:24
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jul 18, 2022

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
Copy Markdown
Contributor

mrambacher commented Jul 19, 2022

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
Copy Markdown
Contributor

riversand963 commented Jul 19, 2022

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
Closed
@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.

4 participants

@ajkr @facebook-github-bot @mrambacher @riversand963