jrmccluskey commented on code in PR #40598:
URL: https://github.com/apache/beam/pull/40598#discussion_r4284576962
##########
sdks/python/apache_beam/io/components/adaptive_throttler.py:
##########
@@ -155,16 +165,18 @@ def __init__(
Args:
window_ms: int, length of history to consider, in ms, to set
- throttling.
+ throttling. Should be much larger than throttle_delay_secs and any
+ retry backoff between attempts, otherwise failures expire from the
+ history before they can influence throttling.
Review Comment:
I don't think bringing up the retry backoff is particularly helpful here
since that's set above this level. It's also kind of difficult to discuss in
terms of a static retry since the framework here uses an exponential backoff;
to guarantee this you'd need a massive window size.
##########
sdks/python/apache_beam/io/components/adaptive_throttler.py:
##########
@@ -173,14 +185,16 @@ def __init__(
window_ms=window_ms, bucket_ms=bucket_ms,
overload_ratio=overload_ratio)
def throttle(self):
- """ Stops request code from advancing while the underlying
- AdaptiveThrottler is signaling to preemptively throttle the request.
- Automatically handles logging the throttling and signaling to the SDK
- harness that the request is being throttled. This should be called in any
- context where a call to a remote service is being contacted prior to the
- call being performed.
+ """ Delays the request by throttle_delay_secs if the underlying
+ AdaptiveThrottler elects to preemptively throttle it, based on the failure
+ rate of recent requests. Automatically handles logging the throttling and
+ signaling to the SDK harness that the request is being throttled.
+
+ This should be called once for each attempt to contact a remote
+ service (including retries), prior to the call being performed. Each call
+ is recorded as one request attempt, and delays the request at most once.
"""
- while self.throttle_request(time.time() * _SECONDS_TO_MILLISECONDS):
+ if self.throttle_request(time.time() * _SECONDS_TO_MILLISECONDS):
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
