WIP lgalloc limiter #32602
WIP lgalloc limiter #32602
Conversation
Signed-off-by: Moritz Hoffmann <[email protected]>
|
|
||
| self.tx | ||
| .send(Update::DiskLimit(Some(disk_limit))) | ||
| .expect("Sender exists"); |
There was a problem hiding this comment.
| .expect("Sender exists"); | |
| .expect("Receiver exists"); |
(Which is what you have on the first send call, and seems more accurate.)
| Interval(Duration), | ||
| DiskLimit(Option<usize>), | ||
| BurstBudget(usize), | ||
| } |
There was a problem hiding this comment.
Sending these as two/three separate messages every time the config changes leads to some odd transient states -- for example, we can briefly have the disk-limit re-enabled but be using a stale value for the burst budget. May not matter, but seems easier to reason about if this were a struct and all configs were updated atomically?
| @@ -354,6 +355,7 @@ async fn run(args: Args) -> Result<(), anyhow::Error> { | |||
| ComputeInstanceContext { | |||
| scratch_directory: args.scratch_directory, | |||
| worker_core_affinity: args.worker_core_affinity, | |||
| announce_memory_limit: args.announce_memory_limit, | |||
There was a problem hiding this comment.
I am confused about this being called announce_memory_limit instead of just memory_limit. Is there a good reason? If not, it's probably too much of a hassle to change the command-line option, but we can change the internal variable names.
| "Multiplicative bias to lgalloc_limiter_usage_factor.", | ||
| ); | ||
|
|
||
| /// Bias to the lgalloc limiter usage factor. |
There was a problem hiding this comment.
| /// Bias to the lgalloc limiter usage factor. | |
| /// Burst factor to disk limit. |
|
|
||
| /// Bias to the lgalloc limiter usage factor. | ||
| pub const LGALLOC_LIMITER_BURST_FACTOR: Config<f64> = Config::new( | ||
| "lgalloc_limiter_BURST_FACTOR", |
There was a problem hiding this comment.
| "lgalloc_limiter_BURST_FACTOR", | |
| "lgalloc_limiter_burst_factor", |
| /// but it might delete previous metrics. If we ever want to change this, we should | ||
| /// remove the shared static mutex and make this function return a handle to the metrics. | ||
| /// | ||
| /// This function is async, because it needs to be called from a tokio runtime context. |
There was a problem hiding this comment.
I'm wondering how much value this pattern adds. You add some noise but needing the async keyword, the allow, and this comment. You get protection against calling this function outside an async (not necessarily tokio!) context, but I think you also have this protection through CI, as spawning a task would immediately panic if no tokio runtime was available.
If we want to use this pattern here, shouldn't Limiter::create_task also be async?
| /// | ||
| /// This function is async, because it needs to be called from a tokio runtime context. | ||
| #[allow(clippy::unused_async)] | ||
| pub async fn register_metrics_into( |
There was a problem hiding this comment.
I think this should be called run_limiter or somesuch. A call to "register metrics" spawning a non-metrics task in the background is surprising. A call to "run limiter" running a limiter that also registers metrics into the provided registry is not.
| // Get lgalloc stats and obtain the disk utilization from file stats, summed across all | ||
| // files and size classes. Compare the disk utilization against the configured disk limit, | ||
| // and if it exceeds the limit, reduce the burst budget by the amount of disk utilization | ||
| // that exceeds the limit. If the burst budget is exhausted, we will not allow any more disk | ||
| // access and terminate the process. |
There was a problem hiding this comment.
Nit: This fits better in the module docstring imo.
| /// Bias to the lgalloc limiter usage factor. | ||
| pub const LGALLOC_LIMITER_BURST_FACTOR: Config<f64> = Config::new( | ||
| "lgalloc_limiter_BURST_FACTOR", | ||
| 0.5, |
There was a problem hiding this comment.
0.5 seems like a too-small value, assuming it means you get "0.5 x limit byte-seconds" of burst. If a hydration takes 10 minute that's less than 0.1% of additional disk you can use.
In any case, we should probably disable bursting by default, and make switching it on a conscious decision. Then we can also decide how much bursting we need based on the use case we want to enable.
There was a problem hiding this comment.
Yep, it should be disabled by default.
| disk_utilization, | ||
| disk_limit | ||
| ); | ||
| std::process::exit(1); |
There was a problem hiding this comment.
We need to check what implications existing with code 1 has. It might trigger alerts in our monitoring or show the replica as crashed due to an error in the console. Ideally we want this case the look the same as an OOD scenario.
Also, we should log a warning, not an error. Errors will produce noise in Sentry we don't want.
Signed-off-by: Moritz Hoffmann [email protected]
Part of MaterializeInc/database-issues/issues/9306
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.