Ram only mode for liquid cache#3
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let victims = self.cache_policy.find_victim(1); | ||
| if victims.is_empty() { | ||
| // no advice, because the cache is already empty | ||
| // this can happen if the entry to be inserted is too large, in that case, | ||
| // we write it to disk | ||
| let on_disk_batch = self | ||
| .write_in_memory_batch_to_disk(entry_id, not_inserted) | ||
| .await; | ||
| batch_to_cache = on_disk_batch; | ||
| continue; | ||
| // Nothing left to free; the entry is larger than the budget. | ||
| return; | ||
| } |
There was a problem hiding this comment.
Two existing tests assume the now-removed squeeze/spill path and will fail under this change:
src/core/src/cache/core.rs:982(test_cache_advice_strategies) doesstore.index().get(&entry_id1).unwrap()and assertsMemoryLiquidafter eviction. Under RAM-only behaviorentry_id1is dropped, sounwrap()panics.src/core/src/cache/tests/policies.rs:26(default_policies) inserts 5 entries into a 2-entry-capacity cache and reads back all 5 — only the last 2 survive now, the earlier reads will returnNoneand the snapshot will diverge.src/core/src/cache/tests/policies.rs:51(insert_wont_fit_cache) inserts an oversized entry and unwraps the read result; under the new code the insert is silently skipped (thereturnhere onvictims.is_empty()), so the read returnsNoneand the test panics. The correspondingliquid_cache__cache__tests__policies__insert_wont_fit_cache.snapis also stale.
Please update or replace these tests to verify the new RAM-only behavior (entries are dropped on eviction; oversized entries are not cached) and refresh the affected snapshot.
|
|
||
| fn drop_victims(&self, victims: &[EntryID]) { | ||
| for victim in victims { | ||
| self.trace(InternalEvent::SqueezeVictim { entry: *victim }); |
There was a problem hiding this comment.
super nit: (not blocking) the trace event is still InternalEvent::SqueezeVictim, but the new path drops the victim rather than squeezing it. Consider renaming the variant (or adding a new DropVictim event) so traces accurately describe what happened.
| } | ||
| } | ||
|
|
||
| fn drop_victims(&self, victims: &[EntryID]) { |
There was a problem hiding this comment.
nit: (not blocking) with the new RAM-only insert path, several pieces of code are no longer reachable: squeeze_victims / squeeze_victim_inner (~lines 411–458), write_in_memory_batch_to_disk (~lines 260–303), and the squeeze_policy field on LiquidCache together with LiquidCacheBuilder::with_squeeze_policy. Either delete them, or — if "RAM-only" is meant to be a configurable mode rather than the only mode — wire up an explicit toggle on the builder. As-is, callers can still pass a SqueezePolicy that silently has no effect, which is misleading.
There was a problem hiding this comment.
Review
Blocking Issues
- Existing tests assume the squeeze/spill behavior and will fail under this change. None of the tests appear to have been updated for the new RAM-only semantics:
src/core/src/cache/core.rs:982—test_cache_advice_strategiesunwrapsindex().get(&entry_id1)after eviction and assertsMemoryLiquid. Under the new codeentry_id1is dropped, so this panics.src/core/src/cache/tests/policies.rs:26—default_policiesinserts 5 entries into a 2-entry cache and reads back all 5; only the last 2 will survive.src/core/src/cache/tests/policies.rs:51—insert_wont_fit_cacheexpects to read an oversized entry that is now silently skipped.src/core/src/cache/tests/snapshots/liquid_cache__cache__tests__policies__insert_wont_fit_cache.snapis now stale.
Action Required
- Update or replace the three tests above to verify the new RAM-only behavior: evicted entries are dropped (not transcoded), and entries that exceed the budget after the cache is empty are not cached.
- Refresh the affected insta snapshot.
- Decide whether
SqueezePolicy/ disk-spill is being removed entirely or made configurable, and either delete the now-dead code (squeeze_victims,squeeze_victim_inner,write_in_memory_batch_to_disk,squeeze_policyfield,LiquidCacheBuilder::with_squeeze_policy) or add an explicit RAM-only toggle on the builder so callers don't pass a squeeze policy that has no effect. - Consider adding a PR description that calls out the user-visible behavior change: oversized inserts and evictions are now silent drops rather than disk spills.
1 participant
No description provided.