refactor(foyer): upgrade foyer to v0.21.0-dev #23750
Conversation
Temporarily remove vector cache stats, restore it after state check is fixed. Signed-off-by: MrCroxx <[email protected]>
MrCroxx
requested review from
BugenZhao,
hzxa21 and
wenym1
and removed request for
a team
|
cc @codex Please review for us. |
![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.
ℹ️ 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".
| match policy { | ||
| CachePolicy::Fill(hint) => { | ||
| let entry = self.block_cache.fetch_with_properties( | ||
| idx, | ||
| HybridCacheProperties::default().with_hint(hint), | ||
| fetch_block, | ||
| ); | ||
| if matches!(entry.state(), FetchState::Miss) { | ||
| stats.cache_data_block_miss += 1; | ||
| } | ||
| Ok(BlockResponse::Entry(entry)) | ||
| let fetch = self.block_cache.get_or_fetch(&idx, move |_| { | ||
| Self::fetch_block_with_hint( | ||
| store, | ||
| object_id, | ||
| data_path, | ||
| file_size, | ||
| range, | ||
| uncompressed_capacity, | ||
| hint, | ||
| ) | ||
| }); | ||
| Ok(BlockResponse::Fetch(fetch)) |
There was a problem hiding this comment.
The refactor to HybridGetOrFetch dropped the only place that bumped stats.cache_data_block_miss. When a data block is absent and CachePolicy::Fill schedules a fetch, the function now immediately returns BlockResponse::Fetch without checking fetch.state() and without updating the miss counter. As a result StoreLocalStatistic.cache_data_block_miss stays at 0 even when every request misses, making cache effectiveness metrics and related alarms meaningless.
Useful? React with 👍 / 👎.
| let add = (now.elapsed().as_secs_f64() * 1000.0).ceil(); | ||
| stats_ptr.fetch_add(add as u64, Ordering::Relaxed); | ||
| Ok(Box::new(sst)) | ||
| } | ||
| }); | ||
|
|
||
| if matches! { entry.state(), FetchState::Wait | FetchState::Miss } { | ||
| stats.cache_meta_block_miss += 1; | ||
| } | ||
| }); | ||
|
|
||
| stats.cache_meta_block_total += 1; | ||
|
|
There was a problem hiding this comment.
In the sstable helper, the prior code incremented cache_meta_block_miss whenever the meta cache returned FetchState::Wait | Miss. After switching to get_or_fetch, that miss accounting was removed and the function only increments cache_meta_block_total. This means the exported metric local_stats.cache_meta_block_miss will permanently report zero, hiding real meta-cache misses and breaking existing monitoring.
Useful? React with 👍 / 👎.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Upgrade foyer to v0.21.0-dev.
Temporarily remove vector cache stats, restore it after state check is fixed. cc @wenym1
Checklist
Documentation
Release note