Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(adaptive_concurrency): support configuring the initial ARC limit #18175

Merged
merged 4 commits into from
Aug 8, 2023
Merged

feat(adaptive_concurrency): support configuring the initial ARC limit #18175

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b439ba7
feat(adaptive_concurrency): support configuring the initial ARC limit
blake-mealey Aug 7, 2023
f0f1d62
docs: update component docs for new arc configuration
blake-mealey Aug 7, 2023
5f20cda
replace option with default value and add range validation
blake-mealey Aug 8, 2023
4978ce9
update docs
blake-mealey Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/sinks/util/adaptive_concurrency/controller.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ impl<L> Controller<L> {
// current limit and the maximum, effectively bypassing all the
// mechanisms. Otherwise, the current limit is set to 1 and the
// maximum to MAX_CONCURRENCY.
let current_limit = concurrency.unwrap_or(1);
let current_limit = concurrency.unwrap_or(settings.initial_concurrency.unwrap_or(1));
Self {
semaphore: Arc::new(ShrinkableSemaphore::new(current_limit)),
concurrency,
Expand Down
9 changes: 9 additions & 0 deletions src/sinks/util/adaptive_concurrency/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ pub(self) fn instant_now() -> std::time::Instant {
#[derive(Clone, Copy, Debug)]
#[serde(deny_unknown_fields)]
pub struct AdaptiveConcurrencySettings {
/// The initial concurrency limit to use. If not specified, the initial limit will be 1 (no concurrency).
blake-mealey marked this conversation as resolved.
Show resolved Hide resolved
///
/// It is recommended to set this value to your service's average limit if you're seeing that it takes a
/// long time to ramp up adaptive concurrency after a restart. You can find this value by looking at the
/// `adaptive_concurrency_limit` metric.
#[configurable(derived)]
pub(super) initial_concurrency: Option<usize>,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After taking another look, there are some small changes we'll want to make to improve the documentation and introduce safe-guards for the user.

  1. Change the type from Option<usize> to usize with a default value of 1. This way the default value of 1 will automatically be included in the documentation, rather than having to specify it in the doc comment, which may go out of sync.
  2. Add a validation range (#[configurable(validation(range(min = 1, max = MAX_CONCURRENCY)))]) to the value to ensure that the user does not set it above MAX_CONCURRENCY.
Suggested change
/// The initial concurrency limit to use. If not specified, the initial limit will be 1 (no concurrency).
///
/// It is recommended to set this value to your service's average limit if you're seeing that it takes a
/// long time to ramp up adaptive concurrency after a restart. You can find this value by looking at the
/// `adaptive_concurrency_limit` metric.
#[configurable(derived)]
pub(super) initial_concurrency: Option<usize>,
/// The initial concurrency limit to use.
///
/// It is recommended to set this value to your service's average limit if you're seeing that it takes a
/// long time to ramp up adaptive concurrency after a restart. You can find this value by looking at the
/// `adaptive_concurrency_limit` metric.
#[configurable(validation(range(min = 1, max = MAX_CONCURRENCY)))]
#[serde(default = "default_initial_concurrency")]
pub(super) initial_concurrency: usize,

Farther down the file, you'll want to add

const fn default_initial_concurrency() -> usize {
    1
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unable to pass MAX_CONCURRENCY (or "MAX_CONCURRENCY") to the validation range, so I instead passed 200 directly and added a comment to the MAX_CONCURRENCY constant that any change to the constant should also be made to this range.

/// The fraction of the current value to set the new concurrency limit when decreasing the limit.
///
/// Valid values are greater than `0` and less than `1`. Smaller values cause the algorithm to scale back rapidly
Expand Down Expand Up @@ -84,6 +92,7 @@ impl AdaptiveConcurrencySettings {
impl Default for AdaptiveConcurrencySettings {
fn default() -> Self {
Self {
initial_concurrency: None,
decrease_ratio: default_decrease_ratio(),
ewma_alpha: default_ewma_alpha(),
rtt_deviation_scale: default_rtt_deviation_scale(),
Expand Down
Loading