feat: mutable barrier interval #8206
Conversation
| fn get_eviction_check_interval(params: &SystemParamsReaderRef) -> u64 { | ||
| // Arbitrarily set a minimal barrier interval in case it is too small, | ||
| // especially when it's 0. | ||
| std::cmp::max(params.load().barrier_interval_ms() as u64, 10) | ||
| } |
There was a problem hiding this comment.
May we print a warning somewhere if the user sets a too-small barrier_interval_ms?
There was a problem hiding this comment.
We can check the value range on ALTER SYSTEM SET, the question is how small is too-small 🤣.
| select! { | ||
| // Barrier interval has changed. | ||
| Ok(_) = params_rx.changed() => { | ||
| let new_barrier_interval_ms = Self::get_eviction_check_interval(¶ms_rx.borrow()); | ||
| if new_barrier_interval_ms != barrier_interval_ms | ||
| { | ||
| tick_interval = tokio::time::interval(Duration::from_millis(new_barrier_interval_ms)); | ||
| barrier_interval_ms = new_barrier_interval_ms; | ||
| } | ||
| } | ||
| _ = tick_interval.tick() => {}, | ||
| } |
There was a problem hiding this comment.
Please don't change this. Make barrier interval too large may lead to oom.
There was a problem hiding this comment.
Shall we have another config for this interval?
There was a problem hiding this comment.
I agree that we should read barrier_interval_ms should come from system parameter and unify the access of it. But whether allowing changing it should be discussed, it may affect many components, such as memory management, and maybe storage. So I would prefer to disable changing it at first.
There was a problem hiding this comment.
The eviction of the LRU cache (i.e. streaming memory control) actually still depends on barriers, so simply having another config for this checking interval may not help. If we do want to make barrier interval configurable, we should make sure our users know what they are doing (including the side effects). cc. @fuyufjh
|
As discussed offline, barrier interval should not be changed 🤣 |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
barrier_interval_msis used in several placesGlobalBarrierManager: schedule barrierGlobalMemoryManager: check eviction every barrier_interval.SourceExecutorFsSourceExecutorI have manually tested setting barrier interval to values ranging from 500ms to 30s, which seems to be working fine.
Checklist For Contributors
./risedev check(or alias,./risedev c)Checklist For Reviewers
Documentation
Click here for Documentation
Types of user-facing changes
Please keep the types that apply to your changes, and remove the others.
Release note