Add public API RateLimiter::GetTotalPendingRequests() #8890

hx235 · 2021-09-08T19:25:53Z

Context/Summary:
As users requested, a public API RateLimiter::GetTotalPendingRequests() is added to expose the total number of pending requests for bytes in the rate limiter, which is the size of the request queue of that priority (or of all priorities, if IO_TOTAL is interested) at the time when this API is called.

Test plan:

Passing added new unit tests
Passing existing unit tests

facebook-github-bot · 2021-09-08T19:39:36Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-09T00:17:12Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-09T00:17:29Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-09T00:24:30Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-09T00:24:41Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-09T00:25:12Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ajkr

LGTM!

ajkr · 2021-09-09T03:04:04Z

util/rate_limiter_test.cc

+        // We temporarily unlock the mutex so that the following
+        // GetTotalPendingRequests() can acquire it
+        request_mutex->Unlock();
+        EXPECT_EQ(limiter->GetTotalPendingRequests(Env::IO_USER), 1);


Good idea to check GetTotalPendingRequests() in the Request() thread so we don't have to worry about race conditions.

Maybe also assert the callback is called so minor changes, like initializing RateLimiter with available_bytes_ = refill_bytes_per_period_, won't defeat the purpose of the test. That could be done by (1) introducing a flag, nonzero_pending_requests_verified_ = false (or better name), before EnableProcessing(); (2) setting nonzero_pending_requests_verified_ = true in the callback; and (3) adding an EXPECT_EQ for nonzero_pending_requests_verified_ to be true after calling Request().

Ahh - that's a good testing habit to make sure callback is called if there is any assertion done in it. I will update this PR and open a task to improve others in this test suit. Thanks!

facebook-github-bot · 2021-09-09T16:25:02Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-09T16:25:25Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-09T16:26:05Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hx235 · 2021-09-09T16:26:38Z

Update:

Added assertion to verify the test callback is indeed called as suggested
Will merge once signals are cleared

facebook-github-bot · 2021-09-09T16:27:36Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-09T19:25:47Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-09T19:34:02Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-09T23:59:16Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-09T23:59:57Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-10T00:00:11Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hx235 · 2021-09-10T00:01:45Z

Update:

Manual rebase

facebook-github-bot · 2021-09-10T00:02:01Z

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-09-10T15:37:12Z

@hx235 merged this pull request in 1254248.

Summary: - Fixed a bug in `RateLimiterTest.GeneratePriorityIterationOrder` that the callbacks in this test were not called starting from `i = 1`. Fix by increasing `rate_bytes_per_sec` and requested bytes. - The bug is due to the previous `rate_bytes_per_sec` was set too small, resulting in `refill_bytes_per_period` less than `kMinRefillBytesPerPeriod`. Hence the actual `refill_bytes_per_period` was equal to `kMinRefillBytesPerPeriod` due to the logic [here](https://github.com/facebook/rocksdb/blob/main/util/rate_limiter.cc#L302-L303) and it ended up being greater than the previously set requested bytes. Therefore starting from `i = 1`, `RefillBytesAndGrantRequests()` and `GeneratePriorityIterationOrder` won't be called and the test callbacks was not triggered to execute the assertion. - Added internal flag to assert callbacks are called in `RateLimiterTest.GeneratePriorityIterationOrder` to prevent any future changes defeat the purpose of the test [as suggested](#8890 (comment)) - Increased `rate_bytes_per_sec` and bytes of each request in `RateLimiterTest.GetTotalBytesThrough`, `RateLimiterTest.GetTotalRequests`, `RateLimiterTest.GetTotalPendingRequests` to trigger the "long path" of execution (i.e, the one trigger RefillBytesAndGrantRequests()) to increase test coverage - This increased the running time of the three tests, see test plan for time difference running locally - Cleared up sync point effects after each test by calling `SyncPoint::GetInstance()->DisableProcessing();` and `SyncPoint::GetInstance()->ClearAllCallBacks();` in `~RateLimiterTest()` [as suggested](https://github.com/facebook/rocksdb/pull/8595/files#r697534279) - It's fine to call these two methods even when `EnableProcessing()` or `SetCallBack()` is not called in the test or is already cleaned up. In those cases, calling these two functions in destructor is effectively no-op. - This will allow cleaning up sync point effects of previous test even when the previous test failed in assertion. - Added missing `SyncPoint::GetInstance()->DisableProcessing();` and `SyncPoint::GetInstance()->ClearCallBacks(..);` in existing tests for completeness - Called `SyncPoint::GetInstance()->DisableProcessing();` and `SyncPoint::GetInstance()->ClearCallBacks(..);` in loop in `RateLimiterTest.GeneratePriorityIterationOrder` for completeness Pull Request resolved: #8904 Test Plan: - Passing existing tests - To verify the 1st change, run `RateLimiterTest.GeneratePriorityIterationOrder` with assertions of callbacks are indeed called under original `rate_bytes_per_sec` and request byte and under updated `rate_bytes_per_sec` and request byte. The former will fail the assertion while the latter succeeds. - Here is the increased test time due to the 3rd change mentioned above in the summary. The relevant 3 tests mentioned in total increase the test time by 6s (~6000/33848 = 17.7% of the original total test time), which IMO is acceptable for better test coverage through running the "long path". - current (run on branch rate_limiter_ut_improve locally) [ RUN ] RateLimiterTest.GetTotalBytesThrough [ OK ] RateLimiterTest.GetTotalBytesThrough (3000 ms) [ RUN ] RateLimiterTest.GetTotalRequests [ OK ] RateLimiterTest.GetTotalRequests (3001 ms) [ RUN ] RateLimiterTest.GetTotalPendingRequests [ OK ] RateLimiterTest.GetTotalPendingRequests (0 ms) ... [----------] 10 tests from RateLimiterTest (43349 ms total) [----------] Global test environment tear-down [==========] 10 tests from 1 test case ran. (43349 ms total) [ PASSED ] 10 tests. - previous (run on branch main locally) [ RUN ] RateLimiterTest.GetTotalBytesThrough [ OK ] RateLimiterTest.GetTotalBytesThrough (0 ms) [ RUN ] RateLimiterTest.GetTotalRequests [ OK ] RateLimiterTest.GetTotalRequests (0 ms) [ RUN ] RateLimiterTest.GetTotalPendingRequests [ OK ] RateLimiterTest.GetTotalPendingRequests (0 ms) ... [----------] 10 tests from RateLimiterTest (33848 ms total) [----------] Global test environment tear-down [==========] 10 tests from 1 test case ran. (33848 ms total) [ PASSED ] 10 tests. Reviewed By: ajkr Differential Revision: D30872544 Pulled By: hx235 fbshipit-source-id: ff894f5c1a4bef70e8e407d53b00be45f776b3e4

…ard compability (#8938) Summary: Context/Summary: #8890 added a public API `RateLimiter::GetTotalPendingRequest()` but mistakenly marked it as pure virtual, forcing RateLimiter's derived classes to implement this function and breaking backward compatibility. This PR makes `RateLimiter::GetTotalPendingRequest()` as non-pure virtual method by providing a trivial implementation in rate_limiter.h Pull Request resolved: #8938 Test Plan: Passing existing tests Reviewed By: pdillinger Differential Revision: D31100661 Pulled By: hx235 fbshipit-source-id: 06eff1005156a6e5a881e393b2c5b2ad706897d8

… default impl (#8950) Summary: Context: After more discussion, a fix in #8938 might turn out to be too restrictive for the case where `GetTotalPendingRequests` might be invoked on RateLimiter classes that does not support the recently added API `RateLimiter::GetTotalPendingRequests` (#8890) due to the `assert(false)` in #8938. Furthermore, sentinel value like `-1` proposed in #8938 is easy to be ignored and unchecked. Therefore we decided to adopt `Status::NotSupported()`, which is also a convention of adding new API to public header in RocksDB. - Changed return value type of `RateLimiter::GetTotalPendingRequests` in related declaration/definition - Passed in pointer argument to hold the output instead of returning it as before - Adapted to the changes above in calling `RateLimiter::GetTotalPendingRequests` in test - Minor improvement to `TEST_F(RateLimiterTest, GetTotalPendingRequests)`: added failure message for assertion and replaced repetitive statements with a loop Pull Request resolved: #8950 Reviewed By: ajkr, pdillinger Differential Revision: D31128450 Pulled By: hx235 fbshipit-source-id: 282ac9c4f3dacaa0aec6d0a993161f77ad47a040

Summary: Context/Summary: As users requested, a public API RateLimiter::GetTotalPendingRequests() is added to expose the total number of pending requests for bytes in the rate limiter, which is the size of the request queue of that priority (or of all priorities, if IO_TOTAL is interested) at the time when this API is called. Pull Request resolved: facebook/rocksdb#8890 Test Plan: - Passing added new unit tests - Passing existing unit tests Reviewed By: ajkr Differential Revision: D30815500 Pulled By: hx235 fbshipit-source-id: 2dfa990f651c1c47378b6215c751ad76a5824300

Summary: - Fixed a bug in `RateLimiterTest.GeneratePriorityIterationOrder` that the callbacks in this test were not called starting from `i = 1`. Fix by increasing `rate_bytes_per_sec` and requested bytes. - The bug is due to the previous `rate_bytes_per_sec` was set too small, resulting in `refill_bytes_per_period` less than `kMinRefillBytesPerPeriod`. Hence the actual `refill_bytes_per_period` was equal to `kMinRefillBytesPerPeriod` due to the logic [here](https://github.com/facebook/rocksdb/blob/main/util/rate_limiter.cc#L302-L303) and it ended up being greater than the previously set requested bytes. Therefore starting from `i = 1`, `RefillBytesAndGrantRequests()` and `GeneratePriorityIterationOrder` won't be called and the test callbacks was not triggered to execute the assertion. - Added internal flag to assert callbacks are called in `RateLimiterTest.GeneratePriorityIterationOrder` to prevent any future changes defeat the purpose of the test [as suggested](facebook/rocksdb#8890 (comment)) - Increased `rate_bytes_per_sec` and bytes of each request in `RateLimiterTest.GetTotalBytesThrough`, `RateLimiterTest.GetTotalRequests`, `RateLimiterTest.GetTotalPendingRequests` to trigger the "long path" of execution (i.e, the one trigger RefillBytesAndGrantRequests()) to increase test coverage - This increased the running time of the three tests, see test plan for time difference running locally - Cleared up sync point effects after each test by calling `SyncPoint::GetInstance()->DisableProcessing();` and `SyncPoint::GetInstance()->ClearAllCallBacks();` in `~RateLimiterTest()` [as suggested](https://github.com/facebook/rocksdb/pull/8595/files#r697534279) - It's fine to call these two methods even when `EnableProcessing()` or `SetCallBack()` is not called in the test or is already cleaned up. In those cases, calling these two functions in destructor is effectively no-op. - This will allow cleaning up sync point effects of previous test even when the previous test failed in assertion. - Added missing `SyncPoint::GetInstance()->DisableProcessing();` and `SyncPoint::GetInstance()->ClearCallBacks(..);` in existing tests for completeness - Called `SyncPoint::GetInstance()->DisableProcessing();` and `SyncPoint::GetInstance()->ClearCallBacks(..);` in loop in `RateLimiterTest.GeneratePriorityIterationOrder` for completeness Pull Request resolved: facebook/rocksdb#8904 Test Plan: - Passing existing tests - To verify the 1st change, run `RateLimiterTest.GeneratePriorityIterationOrder` with assertions of callbacks are indeed called under original `rate_bytes_per_sec` and request byte and under updated `rate_bytes_per_sec` and request byte. The former will fail the assertion while the latter succeeds. - Here is the increased test time due to the 3rd change mentioned above in the summary. The relevant 3 tests mentioned in total increase the test time by 6s (~6000/33848 = 17.7% of the original total test time), which IMO is acceptable for better test coverage through running the "long path". - current (run on branch rate_limiter_ut_improve locally) [ RUN ] RateLimiterTest.GetTotalBytesThrough [ OK ] RateLimiterTest.GetTotalBytesThrough (3000 ms) [ RUN ] RateLimiterTest.GetTotalRequests [ OK ] RateLimiterTest.GetTotalRequests (3001 ms) [ RUN ] RateLimiterTest.GetTotalPendingRequests [ OK ] RateLimiterTest.GetTotalPendingRequests (0 ms) ... [----------] 10 tests from RateLimiterTest (43349 ms total) [----------] Global test environment tear-down [==========] 10 tests from 1 test case ran. (43349 ms total) [ PASSED ] 10 tests. - previous (run on branch main locally) [ RUN ] RateLimiterTest.GetTotalBytesThrough [ OK ] RateLimiterTest.GetTotalBytesThrough (0 ms) [ RUN ] RateLimiterTest.GetTotalRequests [ OK ] RateLimiterTest.GetTotalRequests (0 ms) [ RUN ] RateLimiterTest.GetTotalPendingRequests [ OK ] RateLimiterTest.GetTotalPendingRequests (0 ms) ... [----------] 10 tests from RateLimiterTest (33848 ms total) [----------] Global test environment tear-down [==========] 10 tests from 1 test case ran. (33848 ms total) [ PASSED ] 10 tests. Reviewed By: ajkr Differential Revision: D30872544 Pulled By: hx235 fbshipit-source-id: ff894f5c1a4bef70e8e407d53b00be45f776b3e4

…ard compability (#8938) Summary: Context/Summary: facebook/rocksdb#8890 added a public API `RateLimiter::GetTotalPendingRequest()` but mistakenly marked it as pure virtual, forcing RateLimiter's derived classes to implement this function and breaking backward compatibility. This PR makes `RateLimiter::GetTotalPendingRequest()` as non-pure virtual method by providing a trivial implementation in rate_limiter.h Pull Request resolved: facebook/rocksdb#8938 Test Plan: Passing existing tests Reviewed By: pdillinger Differential Revision: D31100661 Pulled By: hx235 fbshipit-source-id: 06eff1005156a6e5a881e393b2c5b2ad706897d8

… default impl (#8950) Summary: Context: After more discussion, a fix in facebook/rocksdb#8938 might turn out to be too restrictive for the case where `GetTotalPendingRequests` might be invoked on RateLimiter classes that does not support the recently added API `RateLimiter::GetTotalPendingRequests` (facebook/rocksdb#8890) due to the `assert(false)` in facebook/rocksdb#8938. Furthermore, sentinel value like `-1` proposed in facebook/rocksdb#8938 is easy to be ignored and unchecked. Therefore we decided to adopt `Status::NotSupported()`, which is also a convention of adding new API to public header in RocksDB. - Changed return value type of `RateLimiter::GetTotalPendingRequests` in related declaration/definition - Passed in pointer argument to hold the output instead of returning it as before - Adapted to the changes above in calling `RateLimiter::GetTotalPendingRequests` in test - Minor improvement to `TEST_F(RateLimiterTest, GetTotalPendingRequests)`: added failure message for assertion and replaced repetitive statements with a loop Pull Request resolved: facebook/rocksdb#8950 Reviewed By: ajkr, pdillinger Differential Revision: D31128450 Pulled By: hx235 fbshipit-source-id: 282ac9c4f3dacaa0aec6d0a993161f77ad47a040

facebook-github-bot added the CLA Signed label Sep 8, 2021

hx235 requested a review from ajkr September 8, 2021 21:14

hx235 force-pushed the rate_limiter_pending_req_stat branch from cc507d0 to 710c11b Compare September 9, 2021 00:24

ajkr approved these changes Sep 9, 2021

View reviewed changes

hx235 force-pushed the rate_limiter_pending_req_stat branch from b9abf95 to d58a47f Compare September 9, 2021 16:25

hx235 added 6 commits September 9, 2021 16:57

Add public API GetTotalPendingRequests()

33fec3f

Update HISTORY.md for public API change

8bf4172

Make format

681f4c7

Add more assertion

f03bfec

Add assertion to verify callback is called

a91b76a

Make format

168663f

hx235 force-pushed the rate_limiter_pending_req_stat branch from d58a47f to 168663f Compare September 9, 2021 23:59

facebook-github-bot closed this in 1254248 Sep 10, 2021

facebook-github-bot added the Merged label Sep 10, 2021

hx235 mentioned this pull request Sep 10, 2021

[BetterEng][RateLimiter] Improve rate_limiter_test.cc #8904

Closed

hx235 mentioned this pull request Sep 20, 2021

Make RateLimiter::GetTotalPendingRequest() non pure virtual for backward compability #8938

Closed

hx235 mentioned this pull request Sep 22, 2021

Return Status::NotSupported() in RateLimiter::GetTotalPendingRequests default impl #8950

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add public API RateLimiter::GetTotalPendingRequests() #8890

Add public API RateLimiter::GetTotalPendingRequests() #8890

hx235 commented Sep 8, 2021 •

edited

Loading

facebook-github-bot commented Sep 8, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

ajkr left a comment

ajkr Sep 9, 2021

hx235 Sep 9, 2021 •

edited

Loading

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

hx235 commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 10, 2021

hx235 commented Sep 10, 2021

facebook-github-bot commented Sep 10, 2021

facebook-github-bot commented Sep 10, 2021

Add public API RateLimiter::GetTotalPendingRequests() #8890

Add public API RateLimiter::GetTotalPendingRequests() #8890

Conversation

hx235 commented Sep 8, 2021 • edited Loading

facebook-github-bot commented Sep 8, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

ajkr left a comment

Choose a reason for hiding this comment

ajkr Sep 9, 2021

Choose a reason for hiding this comment

hx235 Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

hx235 commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 9, 2021

facebook-github-bot commented Sep 10, 2021

hx235 commented Sep 10, 2021

facebook-github-bot commented Sep 10, 2021

facebook-github-bot commented Sep 10, 2021

hx235 commented Sep 8, 2021 •

edited

Loading

hx235 Sep 9, 2021 •

edited

Loading