fix(compactor): introduce dedicated config for compactor meta cache #19203
Conversation
| @@ -683,6 +683,11 @@ pub struct CacheConfig { | |||
| #[serde(default)] | |||
| #[config_doc(omitted)] | |||
| pub meta_cache_eviction: CacheEvictionConfig, | |||
|
|
|||
| /// Configure the capacity of the meta cache in MB explicitly for compactor. | |||
| // TODO(li0k): `compactor_meta_cache_capacity_mb` is only used for compactor meta cache configuration, it is not involved in the calculation of CN reserved memory, we consider removing it in the future.. | |||
There was a problem hiding this comment.
QQ: Does this field also benefit standalone deployment?
There was a problem hiding this comment.
I am thinking whether we should just not expose a config to overwrite the compactor meta cache capacity. Instead, we can just use min(128MB, node memory * some fix ratio) as the meta cache capactiy.
There was a problem hiding this comment.
Thanks, I took patrick's suggestion and used a calculated rule to determine the compactor's meta cache.
compactor_meta_cache_capacity_mb = min(128MB, avail_memory * 2%). And I restructured the configuration into Hummock/Compactor memory config so that standalone can also benefit.
src/storage/compactor/src/server.rs
Outdated
| @@ -84,10 +84,18 @@ pub async fn prepare_start_parameters( | |||
| let non_reserved_memory_bytes = (system_memory_available_bytes() as f64 | |||
| * config.storage.compactor_memory_available_proportion) | |||
| as usize; | |||
| let meta_cache_capacity_bytes = storage_opts.meta_cache_capacity_mb * (1 << 20); | |||
| let meta_cache_capacity_bytes = storage_opts.compactor_meta_cache_capacity_mb * (1 << 20); | |||
There was a problem hiding this comment.
Given that extract_storage_memory_config is only used by compactor, I think it is better to directly refactor and rename extract_storage_memory_config.
There was a problem hiding this comment.
ok, rename extract_storage_memory_config to extract_storage_memory_config_for_test and add a new function for compactor memory config extract
…nto li0k/storage_fix_compactor_meta_cache
| pub const MAX_META_CACHE_SHARD_BITS: usize = 4; | ||
| pub const MIN_BUFFER_SIZE_PER_SHARD: usize = 256; | ||
| pub const MAX_BLOCK_CACHE_SHARD_BITS: usize = 6; // It means that there will be 64 shards lru-cache to avoid lock conflict. | ||
|
|
||
| pub fn extract_storage_memory_config(s: &RwConfig) -> StorageMemoryConfig { | ||
| pub const COMPACTOR_MEMORY_PROPORTION: f64 = 0.1; |
There was a problem hiding this comment.
Can we file an issue? I suppose more broadly across different services, their memory ratios should sum up to 1.0.
| pub const COMPACTOR_MEMORY_PROPORTION: f64 = 0.1; | ||
|
|
||
| // calculate the compactor meta cache capacity based on capacity and memory proportion | ||
| // compactor_meta_cache_capacity_mb = min(MIN_COMPACTOR_META_CACHE_CAPACITY_MB, memory * MAX_COMPACTOR_META_CACHE_CAPACITY_PROPORTION) |
There was a problem hiding this comment.
Is memory in the docs here referring to memory used by compactor or overall standalone process memory.
There was a problem hiding this comment.
Both compactor and standalone.
- dedicated compactor: extract_compactor_memory_config = 1.0
- standalone compactor: extract_compactor_memory_config = COMPACTOR_MEMORY_PROPORTION = 0. 1
And it doesn't change the original memory ratios.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
introduce dedicated config for compactor meta cache, avoiding misuse of CN's meta cache configuration in environments that lack config map isolation
related to #19245
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.