-
Notifications
You must be signed in to change notification settings - Fork 2k
[ENH] Write to disk on eviction instead of insertion #5974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Switch hybrid cache policy to write-on-eviction Updates the hybrid cache configuration to persist entries to disk during eviction rather than on insertion. This aligns with the stated goal of reducing foreground disk contention by deferring writes to the eviction path. Key Changes• Change Affected Areas• rust/cache/src/foyer.rs This summary was automatically generated by @propel-code-bot |
| .with_metrics_registry(otel_0_27_metrics) | ||
| .with_tracing_options(tracing_options.clone()) | ||
| .with_policy(foyer::HybridCachePolicy::WriteOnInsertion) | ||
| .with_policy(foyer::HybridCachePolicy::WriteOnEviction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Documentation] This change from WriteOnInsertion to WriteOnEviction is a good optimization for insert latency, as it defers disk writes until an item is evicted from the in-memory tier.
This introduces a trade-off: improved performance at the cost of durability. In the case of an unclean shutdown (e.g., a process crash), any data in the memory cache that hasn't been evicted will be lost. The previous WriteOnInsertion policy provided stronger durability against crashes.
This is a reasonable trade-off for a cache, but it's an important change in behavior. To ensure this is clear for future maintainers, could you add a brief note about this durability trade-off to the PR description?
Context for Agents
This change from `WriteOnInsertion` to `WriteOnEviction` is a good optimization for insert latency, as it defers disk writes until an item is evicted from the in-memory tier.
This introduces a trade-off: improved performance at the cost of durability. In the case of an unclean shutdown (e.g., a process crash), any data in the memory cache that hasn't been evicted will be lost. The previous `WriteOnInsertion` policy provided stronger durability against crashes.
This is a reasonable trade-off for a cache, but it's an important change in behavior. To ensure this is clear for future maintainers, could you add a brief note about this durability trade-off to the PR description?
File: rust/cache/src/foyer.rs
Line: 352|
Found 1 test failure on Blacksmith runners: Failure
|
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
None needed
Observability plan
Monitor cache insert latencies
Documentation Changes
None