Skip to content
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

add HighPriCache and LowPriCache interface to implement PrioritizedCache #144

Closed

Conversation

haoxiang47
Copy link

From this issue: tikv/titan#127
add two interface: NewHighPriCache and NewLowPriCache to implement PrioritizedCache

@haoxiang47 haoxiang47 force-pushed the implement-prioritized-cache branch from ddfe481 to f51af8f Compare February 2, 2020 12:25
@yiwu-arbug
Copy link
Collaborator

@haoxiang47 Thanks for update! Comments:

  1. With PrioritizedCache, what we want to achieve is having a single LRUCache as backend, but expose two wrapper for it. When using the HighPriCache wrapper, on insert cache item, always override priority to high; vice-versa for LowPriCache. It doesn't not change underlying high-pri-pool-ratio.
  2. Can you move the change to titan repo (github.com/tikv/titan)? We would like to keep rocksdb change at minimum.

@haoxiang47
Copy link
Author

ok, I will update my code:

  1. from the lru_cache.cc, I find that only in high_pri_pool_ratio_ > 0 && (e->IsHighPri() || e->HasHit() the element can insert to the high priority, so if we do not change the high_pri_pool_ratio_, maybe we can not always insert into the high lru list? or we just want to add a wrapper that all element from this wapper will be overwrite the priority, but do not change the cache origin logic?
  2. yes I will move it to the titan

@yiwu-arbug
Copy link
Collaborator

or we just want to add a wrapper that all element from this wapper will be overwrite the priority, but do not change the cache origin logic?

This is what I want to do.

@yiwu-arbug
Copy link
Collaborator

Closing in favor of tikv/titan#142

@yiwu-arbug yiwu-arbug closed this Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants