cloud_storage: Fix download throttling ducktape test #17455
Conversation
Lazin
changed the title
Lazin
force-pushed
the
fix/throttling-e2e-test
branch
from
c55ef87 to
7c939d8
Compare
src/v/utils/token_bucket.h
Outdated
| @@ -62,7 +73,21 @@ class token_bucket { | |||
| _refresh_timer.arm(refresh_interval - elapsed); | |||
| } | |||
|
|
|||
| return _sem.wait(as, size); | |||
| auto deficiency = size - _sem.available_units(); | |||
There was a problem hiding this comment.
did you mean available_units here vs one of the other semaphore interfaces for querying units? i ask because this one can become negative depending on how the semaphore is used.
There was a problem hiding this comment.
in this context it should go negative but still, I think that the deficiency value could be incorrect in case of concurrency, I'll update this
|
separate fix #17474 |
Implement 'maybe_throttle' method. The method works the same way as 'throttle'. It returns an optional value which is set to nullopt if there was no throttling. If the throttling was applied it returns the deficiency value (number of units we had to wait for) and the sleep duration.
Previously, the throttling metric was computed before calling 'throttle'. If number of available units was lower than the number of bytes to read we caclulated deficiency value as a difference between the two. In case of concurrency this could lead to incorrect value of the metric. If two fibers will do this check and find out the available untis are too low.
Lazin
force-pushed
the
fix/throttling-e2e-test
branch
from
7c939d8 to
285c8e0
Compare
|
Updated the token_bucket implementation. Now it returns only the amount of time that the bucket was waiting for units. |
| _parent._read_path_probe.download_throttled( | ||
| buffer_size - available); | ||
| auto throttled = co_await _parent._throughput_limit.maybe_throttle( | ||
| buffer_size, _as); | ||
| if (throttled) { | ||
| // This path is taken only when the download is throttled. In case | ||
| // of concurrency every `maybe_throttle` call will return the | ||
| // precise value for deficiency and sleep time. | ||
| auto duration | ||
| = std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| throttled.value()); | ||
| _parent._read_path_probe.download_throttled(duration.count()); |
There was a problem hiding this comment.
What are the units for the parameter passed to download_throttled? It looks like this switches from bytes to milliseconds. That's probably worth explaining in the commit message?
There was a problem hiding this comment.
I updated the description of the metric. It's not possible to reliably capture the metric in bytes (and such metric doesn't make much sense tbh).
Fix the throttling ducktape test. The test was failing because additional restart reset the metric.
Additionally, the PR improves the way the metric is collected and updates the token bucket implementation to improve observability.
Fixes #14139
Backports Required
Release Notes