Repository navigation
fix(cache-moka): bound the caches by bytes instead of entry count - #3164
jackylee-ch wants to merge 2 commits into
Conversation
DEFAULT_CACHE_SIZE_BYTES went straight to Cache::new, but without a weigher moka reads max_capacity as a number of entries, so the 32MiB budget admitted 33_554_432 manifests. Add the weigher iceberg::io::ObjectCache already uses.
There was a problem hiding this comment.
🟡 Changes recommended
The new weigher currently uses a truncating as u32 cast that can under-weigh large values on overflow, weakening the intended byte-budget bound.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes the iceberg-cache-moka integration to ensure the default Moka caches are bounded by a byte budget (as documented) rather than by entry count, by introducing a weigher and switching construction to Cache::builder().max_capacity(..).
Changes:
- Add
byte_bounded_cachehelper that applies asize_of_val-based Moka weigher and enforcesmax_capacityas bytes. - Update
MokaObjectCacheProvider::new()to build both default caches viabyte_bounded_cache. - Add unit tests validating the cache’s weighted sizing behavior and default provider capacity configuration.
File summaries
| File | Description |
|---|---|
| crates/integrations/cache-moka/src/lib.rs | Switch default caches to byte-weighted Moka caches and add tests verifying weight semantics and capacity. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fn byte_bounded_cache<V>(max_capacity_bytes: u64) -> moka::sync::Cache<String, Arc<V>> | ||
| where V: Send + Sync + 'static { | ||
| moka::sync::Cache::builder() | ||
| .weigher(|_, value: &Arc<V>| size_of_val(value.as_ref()) as u32) |
Review feedback: a truncating cast could wrap for a hypothetical value larger than u32::MAX. Unreachable for Manifest (72 bytes) and ManifestList (24 bytes), but the helper is generic, so saturate instead.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
Which issue does this PR close?
None — filed directly. Related to but distinct from #1720, which is about the accuracy of the weigher
iceberg::io::ObjectCachealready has; this crate has no weigher at all.What changes are included in this PR?
DEFAULT_CACHE_SIZE_BYTES(32MiB) was passed straight tomoka::sync::Cache::new. Without aweigher
mokatreatsmax_capacityas a number of entries, so both caches admitted 33_554_432manifests rather than 32MiB of them — the documented budget never bound.
Build them through
Cache::builder().weigher(..).max_capacity(..), using the samesize_of_valweigher asiceberg::io::ObjectCache. No public signature changes.Are these changes tested?
Yes —
test_cache_weighs_entries_by_size_not_countinserts two 512-byte values and assertsweighted_size() == 1024; on the parent commit it is2, the entry count.cargo test --release -p iceberg-cache-moka→ 2 passed.AI Disclosure
Written with AI assistance (Claude Code); I reviewed the change and ran the tests above.
Worth flagging:
size_of_valonArc<Manifest>counts the struct, not the heap its entries own, sothe cap still under-counts — that is exactly what #1720 tracks. This PR only makes the budget a byte
budget; improving the weight function belongs with that issue.