Conversation
Based on quickwit-oss#6700. Pin Foyer and add the disabled-by-default cache configuration, including clean-block and write-throughput defaults. Co-authored-by: Cursor <cursoragent@cursor.com>
Based on quickwit-oss#6701. Encode cache keys portably while rejecting only I/O failures and non-UTF-8 URIs. Co-authored-by: Cursor <cursoragent@cursor.com>
Based on quickwit-oss#6702. Build and recover the process-wide cache, flush write-on-eviction entries on close, and apply configured reclaim and write throttling. Co-authored-by: Cursor <cursoragent@cursor.com>
Based on quickwit-oss#6707. Add the storage decorator, manage cache lifecycle in searchers, and route footer and body range reads through Foyer without changing whole-split cache behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Based on quickwit-oss#6709. Export cache metrics, document production sizing, and disable the long-lived fast-field RAM cache by default when Foyer is configured. Co-authored-by: Cursor <cursoragent@cursor.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 053cb917b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let uri_len = read_u64(reader)? as usize; | ||
| let mut uri_bytes = vec![0; uri_len]; |
There was a problem hiding this comment.
When recovering a truncated or corrupted cache image, the first eight key bytes can decode to an arbitrarily large u64; casting that value and immediately allocating vec![0; uri_len] can exhaust memory or abort the process instead of returning a Foyer decode error so recovery can reject the entry. Validate the conversion and bound the URI length against the encoded entry size before allocating.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
| _buckets: Vec<f64>, | ||
| ) -> BoxedHistogramVec { | ||
| self.register_histogram_vec(name, desc, label_names) |
There was a problem hiding this comment.
When Foyer registers a histogram through register_histogram_vec_with_buckets, this implementation discards the supplied boundaries and creates an ordinary recorder histogram instead. The Prometheus exporter will consequently aggregate these observations using its default buckets rather than Foyer's requested buckets, making the resulting latency and operation distributions inaccurate; propagate the boundaries into the recorder configuration rather than ignoring them.
Useful? React with 👍 / 👎.
| # compression: lz4 | ||
| # recover_mode: quiet | ||
| # block_size: 64M | ||
| # max_entry_size: 60M |
There was a problem hiding this comment.
a full float field will use 8byte per element
We target 10M docs, but we overshoot sometimes, so we may have 12M docs.
So the max size is closer to 100MB
what happens if the size is larger than max_entry_size?
There was a problem hiding this comment.
it is just omitted. Foyer doesn't put any entries that is larger than the max_entry_size to disk. Do you think if we need to update to 128MB? From the bench tests around 1,2,4 QPS, there are total 5.8k entries that is rejected due to larger than the max_entry_size. https://dd.datad0g.com/dashboard/biv-7rp-k3i/byoc-logs-customer-telemetry-foyer?fromUser=true&fullscreen_end_ts=1789399115777&fullscreen_paused=true&fullscreen_refresh_mode=paused&fullscreen_section=overview&fullscreen_start_ts=1789396752165&fullscreen_widget=2364184038667261&graph-explorer__tile_def=N4IgbglgXiBcIBcIFsCmBnVAnCGQBoQtUBHAVwwXTgG1R0EBPAG1TlAAcBDVhBN%2BABMA9gHMA%2BgGNmXdOgiSCiRhwEh0w5hEFKA7toQALOCAB2wrMh4gAvoSaqTAIy5ZqhAGYXkZGdVh0IDwQss4qspjoAPTuIF6Wvlwm5NiMAIy2ALqEKTh4AaCCXAhc4hpkWJJqaAg4krGmXGjJFFjpSrmMJuhkyLDkCgDW%2BghlCBZcoqhlHFqjWFymU%2BKCEOiDUlyShtNcgshr8sKm4k7hchji4yXMwAAkFhLa%2BHfSZAzY4s935oLT2jYAARnQHAYiyY42AB0sikwjIpgQAAoAJRZQjEdAcY6YcTxKwIExINCYPLUGyZGxAA&refresh_mode=sliding&tpl_var_cluster_id%5B0%5D=yahoo-cong-foyer-small-18&from_ts=1789394453004&to_ts=1789408853004&live=true
| /// Disabled-by-default searcher disk cache for exact split byte ranges. | ||
| #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct SplitRangeDiskCacheConfig { |
There was a problem hiding this comment.
I would avoid putting to many foyer specific parameters in the config. We can have defaults and override them via env parameters instead.
There was a problem hiding this comment.
sounds good, once I verify some settings, I will remove them from the config.
| fn default() -> Self { | ||
| SearcherConfig { | ||
| fast_field_cache: CacheConfig::default_with_capacity(ByteSize::gb(1)), | ||
| fast_field_cache: None, |
There was a problem hiding this comment.
why did you change the default here?
There was a problem hiding this comment.
So the whole logic for changing the fast field cache is that we want to keep the existing fast field configuration when there is no disk cache defined. But when disk cache is defined, we want to remove fast field.
so the fast field cache looks like below after the changes:
omitted: disable it when Foyer is configured
explicitly configured: preserve the user’s value
omitted without Foyer: retain the effective 1 GB default
Clarify cache naming and documentation, centralize zero-capacity handling in QuickwitCache, and document the manual key codec accurately.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47b0b6743e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[async_trait] | ||
| impl Storage for FoyerSplitRangeStorage { |
There was a problem hiding this comment.
When a caller invokes list on the returned Arc<dyn Storage>, this implementation inherits Storage::list's default unsupported-operation error instead of listing through inner. Since this public read-only decorator otherwise forwards non-cached reads and metadata operations, wrapping a storage that supports listing unexpectedly removes that capability; add a list implementation that delegates to self.inner.list(prefix).
Useful? React with 👍 / 👎.
| let storage_with_split_range_cache: Arc<dyn Storage> = match &searcher_context | ||
| .split_range_disk_cache_opt | ||
| { | ||
| Some(cache) => wrap_storage_with_split_range_cache(cache.clone(), index_storage.clone()), | ||
| None => index_storage.clone(), |
There was a problem hiding this comment.
When both storage_timeout_policy and the split-range disk cache are enabled, configure_storage_retries wraps only index_storage, and this newly added outer wrapper performs Foyer lookups before reaching it. A stalled Foyer disk read therefore bypasses the configured per-read timeout and retries, so searches wait until the broader leaf-request timeout instead of failing open to remote storage at the configured deadline; enforce the timeout around the cache lookup as well.
Useful? React with 👍 / 👎.
25063ec to
df68cb7
Compare
| subsystem: "storage", | ||
| ); | ||
|
|
||
| fn foyer_histogram_buckets(name: &str) -> Option<Vec<f64>> { |
There was a problem hiding this comment.
I don't understand this at all. Why use a string as the key?
There was a problem hiding this comment.
str here is just the metric name. Foyer’s RegistryOps API provides only: name: Cow<'static, str>. Therefore, we must inspect a string to identify the histogram. We could use some helper functions to replace this but we couldn't completely eliminate the string.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f94f458af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
https://github.com/quickwit-oss/quickwit/blob/81cf59d054cf151bed135668cbeb77d2039f6ebc/quickwit-storage/src/split_range_cache/storage.rs#L69-L70
When max_entry_size is configured close to block_size, an incompressible payload can pass this value-length check while its encoded Foyer entry cannot fit: Foyer 0.22.3 also reserves a 4 KiB blob index and page-aligns the header, key, and value. Such entries are sent to the disk tier but rejected there, causing fail-open reads or repeated remote misses despite being advertised as cacheable; account for the full encoded size and mark non-fitting values InMem, and document this format constraint.
AGENTS.md reference: AGENTS.md:L126-L126
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # disk_capacity: 1500G | ||
| # memory_capacity: 15G | ||
| # memory_eviction_policy: s3-fifo | ||
| # compression: lz4 |
There was a problem hiding this comment.
Is this effective as default for this kind of data? I would assume we will mostly store data that has already some kind of compression and we will just waste CPU for very small size gains.
4 participants
Summary
Test plan
make fmtcargo test -p quickwit-config --lib node_config::tests::test_split_range_disk_cache -- --nocapturecargo test -p quickwit-storage --all-features --lib split_range_cache -- --nocapturecargo test -p quickwit-search --all-features --lib split_range_cache_layer -- --nocapturecargo clippy -p quickwit-config -p quickwit-storage -p quickwit-search --all-features --tests -- -D warningsMade with Cursor