Repository navigation
Conversation
|
R: @jrmccluskey |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
tvalentyn
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40598 +/- ##
=============================================
+ Coverage 56.16% 71.49% +15.32%
- Complexity 2288 13992 +11704
=============================================
Files 1124 2217 +1093
Lines 178336 191033 +12697
Branches 1489 11310 +9821
=============================================
+ Hits 100170 136570 +36400
+ Misses 75645 50255 -25390
- Partials 2521 4208 +1687
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| """ | ||
| while self.throttle_request(time.time() * _SECONDS_TO_MILLISECONDS): | ||
| if self.throttle_request(time.time() * _SECONDS_TO_MILLISECONDS): |
There was a problem hiding this comment.
I don't think delaying a request at most one time is the correct behavior here. Each iteration in the while() loop is a separate attempt and the throttling behavior is a local rejection, not just a time delay before forwarding the request to the service. At steady state with a throttle probability of 1.0 you're not really rejecting any requests locally, just delaying them by the throttle delay before sending them to the service anyway. At that point you might as well just fail fast and have the exponential backoff on retries.
There was a problem hiding this comment.
I don't follow why delaying the request each time counts as a separate request - doesn't it artificially inflate the volume of the requests?
There was a problem hiding this comment.
is it correct to say that with the while loop approach, each decision to throttle (delay) a request during the while loop iteration increases the probability that the attempt on the following iteration of the loop will also be throttled?
There was a problem hiding this comment.
That is correct, and that is intended. To quote the overview of the algorithm:
"As the client itself starts rejecting requests, requests will continue to exceed accepts. While it may seem counterintuitive, given that locally rejected requests aren't actually propagated to the backend, this is the preferred behavior. As the rate at which the application attempts requests to the client grows (relative to the rate at which the backend accepts them), we want to increase the probability of dropping new requests."
There was a problem hiding this comment.
interesting, yes, this is not intuitive. Ok, i reverted this change. Altought seems like we might not be consistent across SDKs on this.
2 participants
Change the sample period defaults to 2 min, since the previous 1 second default was to short to register a service outage: a 5 seconds retry delay wiped out the time horizon over which failed requests were collected, so throttling never kicked in.
Note: Python's AdaptiveThrottler checks whether to throttle multiple times per attempt, while the Java ReactiveThrottler seems to only throttle once.