8000 storage: revisit TimestampCache eviction policy · Issue #14518 · cockroachdb/cockroach · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

storage: revisit TimestampCache eviction policy #14518

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.

Sign up for GitHub

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

Jump to bottom
Closed
petermattis opened this issue Mar 31, 2017 · 2 comments
Closed

storage: revisit TimestampCache eviction policy #14518

petermattis opened this issue Mar 31, 2017 · 2 comments
Assignees
@nvanbenschoten @petermattis
Milestone
Later

Comments

@petermattis
Copy link
Collaborator

Internally, the TimestampCache has 3 caches: a read interval cache, a write interval cache and a request cache. The combined size of the caches is limited to TimestampCache.maxBytes, but the eviction from the caches is funky: we only evict from one of the sub-caches when inserting into it. For example, TimestampCache.AddRequest is the only place we evict from the request cache.

I'm not certain that this behavior is problematic, though it seems odd that inserting into the read interval cache might push TimestampCache.bytes over the maxBytes threshold, but not result in anything being evicted because all of the memory is consumed in the write interval cache.
@petermattis petermattis added this to the Later milestone Mar 31, 2017
@petermattis petermattis self-assigned this Mar 31, 2017
@petermattis
Copy link
Collaborator Author

We could change TimestampCache.{r,w}cache into interval trees and perform the cache maintenance and eviction in TimestampCache. Might be doable cleanly via some reorganization of cache.baseCache, though I haven't thought about it too much. This doesn't seem particularly urgent, though.

@nvanbenschoten
Copy link
Contributor

Closed by #20232.

@nvanbenschoten nvanbenschoten mentioned this issue Nov 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nvanbenschoten nvanbenschoten

@petermattis petermattis

Labels
None yet
Projects
None yet
Milestone
Later
Development

No branches or pull requests
2 participants
@nvanbenschoten @petermattis
0