-
Notifications
You must be signed in to change notification settings - Fork 89
rate-limiting: Add ScheduledStartingRateLimiter #281
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
Changes from all commits
c35d7c8
26e8f00
de66b5e
0058216
41c0d6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,58 @@ TEST_F(RateLimiterTest, BurstingRateLimiterTest) { | |
| EXPECT_FALSE(rate_limiter->tryAcquireOne()); | ||
| } | ||
|
|
||
| TEST_F(RateLimiterTest, ScheduledStartingRateLimiterTest) { | ||
| Envoy::Event::SimulatedTimeSystem time_system; | ||
| const auto schedule_delay = 10ms; | ||
| // We test regular flow, but also the flow where the first aquisition attempt comes after the | ||
| // scheduled delay. This should be business as usual from a functional perspective, but internally | ||
| // this rate limiter specializes on this case to log a warning message, and we want to cover that. | ||
| for (const bool starting_late : std::vector<bool>{false, true}) { | ||
| const Envoy::MonotonicTime scheduled_starting_time = | ||
| time_system.monotonicTime() + schedule_delay; | ||
| std::unique_ptr<MockRateLimiter> mock_rate_limiter = std::make_unique<MockRateLimiter>(); | ||
| MockRateLimiter& unsafe_mock_rate_limiter = *mock_rate_limiter; | ||
| InSequence s; | ||
|
|
||
| EXPECT_CALL(unsafe_mock_rate_limiter, timeSource) | ||
| .Times(AtLeast(1)) | ||
| .WillRepeatedly(ReturnRef(time_system)); | ||
| RateLimiterPtr rate_limiter = std::make_unique<ScheduledStartingRateLimiter>( | ||
| std::move(mock_rate_limiter), scheduled_starting_time); | ||
| EXPECT_CALL(unsafe_mock_rate_limiter, tryAcquireOne) | ||
| .Times(AtLeast(1)) | ||
| .WillRepeatedly(Return(true)); | ||
|
|
||
| if (starting_late) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if starting_late is adding any extra coverage for this test case. This test should already be testing what happens when enough time has passed and when not enough time has passed.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I follow; the intent here is to skip the scheduled delay entirely, so the first call to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, okay. I didn't process the comments in line 74-76 properly when I read this code the first time. I follow now. Is there a way to assert that the log line was actually hit? Or is this just for line code coverage?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just for coverage. Unfortunately we can't really mock the logging today and I couldn't find an efficient way to assert on hitting that line |
||
| time_system.sleep(schedule_delay); | ||
| } | ||
|
|
||
| // We should expect zero releases until it is time to start. | ||
| while (time_system.monotonicTime() < scheduled_starting_time) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this test case doesn't succeed, it will run infinitely rather than fail. Can you add in a tripping mechanism?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The simulated time system here is guaranteed to move forward in this loop with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind here. I was overcomplicated the situation in my head. Sorry about that! |
||
| EXPECT_FALSE(rate_limiter->tryAcquireOne()); | ||
| time_system.sleep(1ms); | ||
| } | ||
|
|
||
| // Now that is time to start, the rate limiter should propagate to the mock rate limiter. | ||
| EXPECT_TRUE(rate_limiter->tryAcquireOne()); | ||
| } | ||
| } | ||
|
|
||
| TEST_F(RateLimiterTest, ScheduledStartingRateLimiterTestBadArgs) { | ||
| Envoy::Event::SimulatedTimeSystem time_system; | ||
| // Verify we enforce future-only scheduling. | ||
| for (const auto timing : std::vector<Envoy::MonotonicTime>{time_system.monotonicTime(), | ||
| time_system.monotonicTime() - 10ms}) { | ||
| std::unique_ptr<MockRateLimiter> mock_rate_limiter = std::make_unique<MockRateLimiter>(); | ||
| MockRateLimiter& unsafe_mock_rate_limiter = *mock_rate_limiter; | ||
| EXPECT_CALL(unsafe_mock_rate_limiter, timeSource) | ||
| .Times(AtLeast(1)) | ||
| .WillRepeatedly(ReturnRef(time_system)); | ||
| EXPECT_THROW(ScheduledStartingRateLimiter(std::move(mock_rate_limiter), timing); | ||
| , NighthawkException); | ||
| } | ||
| } | ||
|
|
||
| class BurstingRateLimiterIntegrationTest : public Test { | ||
| public: | ||
| void testBurstSize(const uint64_t burst_size, const Frequency frequency) { | ||
|
|
||
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.
I'm probably missing something, but couldn't a cancel operation (not yet implemented) result in releasing before the time arrives? What are we trying to prevent by failing here?
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.
Right now, it is only valid to release what has been acquired. So when we hit this, it ought to be impossible to have a preceding successful acquisition because we're not due to start yet. The intent is to catch programming mistakes early when consuming this, but possibly this is overly paranoid.. shall I remove this?
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 explanation makes sense to me. I don't think it's necessary to remove this.