Add support to disable record cache #2034
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AhmedSoliman
requested changes
Oct 4, 2024
| @@ -122,7 +122,12 @@ impl<T: TransportConnect> ReplicatedLogletProvider<T> { | |||
| _metadata_store_client: metadata_store_client, | |||
| networking, | |||
| // todo(asoli): read memory budget from ReplicatedLogletOptions | |||
There was a problem hiding this comment.
should we remove the todo then?
| .max_capacity(memory_budget_bytes.try_into().unwrap_or(u64::MAX)) | ||
| .eviction_policy(EvictionPolicy::lru()) | ||
| .build(); | ||
| /// Creates a new instance of RecordCache. If memory budges is None |
There was a problem hiding this comment.
Suggested change
| /// Creates a new instance of RecordCache. If memory budges is None | |
| /// Creates a new instance of RecordCache. If memory budget is None |
crates/types/src/config/bifrost.rs
Outdated
| @@ -237,6 +237,13 @@ pub struct ReplicatedLogletOptions { | |||
| /// | |||
| /// Retry policy for log server RPCs | |||
| pub log_server_retry_policy: RetryPolicy, | |||
|
|
|||
| /// records_cache_memory_budget_bytes | |||
There was a problem hiding this comment.
Suggested change
| /// records_cache_memory_budget_bytes | |
| /// # In-memory RecordCache memory limit |
crates/types/src/config/bifrost.rs
Outdated
| /// Optional size of records cache in bytes. | ||
| /// If set to None, records cache will be disabled. | ||
| /// Defaults: 20M | ||
| pub records_cache_memory_budget_bytes: Option<usize>, |
There was a problem hiding this comment.
Suggested change
| pub records_cache_memory_budget_bytes: Option<usize>, | |
| pub record_cache_memory_size: Option<usize>, |
There was a problem hiding this comment.
Can you look at common.rs's configuration for inspiration? For configuration keys that accept bytes, we have some helpers to allow setting those values from human-friendly strings. A good example is rocksdb_total_memory_size
crates/types/src/config/bifrost.rs
Outdated
|
|
||
| /// records_cache_memory_budget_bytes | ||
| /// | ||
| /// Optional size of records cache in bytes. |
There was a problem hiding this comment.
Suggested change
| /// Optional size of records cache in bytes. | |
| /// Optional size of record cache in bytes. |
crates/types/src/config/bifrost.rs
Outdated
| @@ -257,6 +264,7 @@ impl Default for ReplicatedLogletOptions { | |||
| Some(10), | |||
| Some(Duration::from_millis(2000)), | |||
| ), | |||
| records_cache_memory_budget_bytes: Some(20_000_000), // 20MB | |||
There was a problem hiding this comment.
Suggested change
| records_cache_memory_budget_bytes: Some(20_000_000), // 20MB | |
| record_cache_memory_size: Some(20_000_000), // 20MB |
muhamadazmy
force-pushed
the
pr2034
branch
2 times, most recently
from
711100a to
ab2bf21
Compare
AhmedSoliman
reviewed
Oct 7, 2024
crates/types/src/config/bifrost.rs
Outdated
Comment on lines
244
to
247
| /// If set to None, record cache will be disabled. | ||
| /// Defaults: 20M | ||
| #[cfg_attr(feature = "schemars", schemars(with = "NonZeroByteCount"))] | ||
| pub record_cache_memory_size: Option<NonZeroByteCount>, |
There was a problem hiding this comment.
toml format doesn't support nulls, so we can't unset this from the configuration file. Perhaps we should allow this to be disabled by setting the size to zero instead.
There was a problem hiding this comment.
Ah okay, good point. I wasn't sure if I should use None, or 0 as the value to disable the cache.
Summary: Since record cache will be used in multiple places. It's possible to disable it by setting the memory budget to None. This way code can still use the cache normally but no records will be cached. All calls to Get will always return None if cache is disabled
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add support to disable record cache
Summary:
Since record cache will be used in multiple places. It's possible
to disable it by setting the memory budget to None. This way code
can still use the cache normally but no records will be cached.
All calls to Get will always return None if cache is disabled