-
Notifications
You must be signed in to change notification settings - Fork 600
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.
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
fix: reject invalid storage cache capacities configs #19016
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed an unrelated thing: the rustdoc of the fields in CacheConfig
and CacheEvictionConfig
are missing. These comments are used to generate a human-readable document via ./risedev generate-example-config
. Please complete these rustdoc @MrCroxx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the following changes in this PR:
- Add
#[config_doc(nested)]
forstorage.cache
,storage.data_file_cache
,storage.meta_file_cache
,storage.cache_refill
so that the relevantCacheConfig
,FileCacheConfig
andCacheRefillConfig
show up indocs.md
. - Add
#[config_doc(omitted)]
forstorage.cache.block_cache_eviction
andstorage.cach.meta_cache_eviction
because I think this is mostly used by us to test the eviction algorithm. @MrCroxx Do you think we need to exposeCacheEvictionConfig
in docs? - Add docs for
CacheConfig
.
@MrCroxx Can you help to add docs for FileCacheConfig
and CacheRefillConfig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Is it okay that I open another PR than merge to this one, or open another PR separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate PR would be better, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Is it okay that I open another PR than merge to this one, or open another PR separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM
add warn log fix typo fix doc fix check fix typo fix log fix log fix docs.md fix test_storage_memory_config fmt
14bf20b
to
4432929
Compare
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
block_cache_capacity_mb
,meta_cache_capacity_mb
,shared_buffer_capacity_mb
are all overwritten via config and their sum <=non_reserved_memory_bytes * storage_memory_portation (default = 0.3)
, will these configs be used.resvered_memory_bytes >= total_memory_bytes
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.