Repository navigation
cache: avoid repeated multi-level capability checks - #2910
codeandsolder wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2910 +/- ##
==========================================
- Coverage 78.13% 75.40% -2.74%
==========================================
Files 74 74
Lines 43570 43637 +67
==========================================
- Hits 34043 32904 -1139
- Misses 9527 10733 +1206 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d3b2f5d to
669e5a8
Compare
| Ok(()) | ||
| } | ||
|
|
||
| fn record_write_result( |
There was a problem hiding this comment.
the L0 branch and write_remaining_levels_async still do the same stats bookkeeping by hand, could they use this too?
| if result.is_err() { | ||
| inc_stat!(stats, write_failures, 1); | ||
| } | ||
| result.with_context(|| format!("failed to write to cache level {idx}"))?; |
There was a problem hiding this comment.
the server logs this with {} (server.rs:1705), so only "failed to write to cache level N" shows up now, without the cause.
could you please keep the cause in the message?
| result.with_context(|| format!("failed to write to cache level {idx}"))?; | ||
|
|
||
| inc_stat!(stats, writes, 1); | ||
| let duration_nanos = u64::try_from(duration.as_nanos()).unwrap_or(u64::MAX); |
There was a problem hiding this comment.
the other places still use as u64, please do the same everywhere
| if let Some(l0) = self.levels.first() { | ||
| // Check if L0 is read-only before attempting write | ||
| if matches!(l0.check().await, Ok(CacheMode::ReadOnly)) { | ||
| if matches!(self.level_mode(0).await?, CacheMode::ReadOnly) { |
There was a problem hiding this comment.
before, a failing check still tried the L0 write; now the put fails, and since errors aren't cached the next write probes again.
is that on purpose?
| } | ||
| } | ||
|
|
||
| async fn assert_check_is_readwrite(storage: &MultiLevelStorage) { |
There was a problem hiding this comment.
this refactor of the existing tests isn't related, please drop it from this PR
669e5a8 to
581be09
Compare
| } | ||
| } | ||
|
|
||
| async fn write_level_mode(&self, idx: usize) -> CacheMode { |
There was a problem hiding this comment.
level_mode and write_level_mode are almost the same, could they be one function?
581be09 to
4b3f91f
Compare
| } | ||
|
|
||
| async fn level_mode(&self, idx: usize, tolerate_check_failure: bool) -> Result<CacheMode> { | ||
| if let Some(mode) = self.level_modes[idx].get() { |
There was a problem hiding this comment.
two concurrent first writes will both run check() here.
could it be one get_or_try_init instead? shorter too
|
|
||
| match self.levels[idx].check().await { | ||
| Ok(mode) => Ok(*self.level_modes[idx].get_or_init(|| async { mode }).await), | ||
| Err(error) if !tolerate_check_failure && self.check_failure_is_fatal(idx) => Err(error), |
There was a problem hiding this comment.
the old "Error checking required cache level N" warning is gone, so this error no longer says which level failed.
could you please add the level as context?
|
|
||
| /// Write to levels starting from `start_idx` asynchronously | ||
| async fn write_remaining_levels_async(&self, key: &str, data: &Bytes, start_idx: usize) { | ||
| async fn write_remaining_levels_async( |
There was a problem hiding this comment.
with true, level_mode never returns Err, so this Result and the ? at the callers are dead, no?
| .unwrap(); | ||
| }); | ||
|
|
||
| assert_eq!(l0.check_count(), 1); |
There was a problem hiding this comment.
please also check that both entries actually ended up in l0
a7ce19a to
becbbba
Compare
Fixes #2818.
Summary
CacheModein an asyncOnceCellStorage::check()per writeA normal server startup still performs the existing storage capability check before serving. Required probe failures keep their existing startup-failure behavior; tolerated failures remain best-effort.
Validation
cargo fmtandgit diff --check