-
Notifications
You must be signed in to change notification settings - Fork 45
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
Eviction callback #31
Conversation
Co-authored-by: Derrick Gilland <dgilland@gmail.com>
Usageset cache = Cache(on_delete=on_delete)
Callable[[key: Hashable, value: Any, cause: EvictedCause], None] First parameter is the key of cache entry has been deleted. Second is the value. Third is the cause of cache entry eviction.
class EvictedCause(Enum):
EXPLICIT = auto()
REPLACED = auto()
EXPIRED = auto()
SIZE = auto()
TriggerThe table shows methods will trigger
⚠ |
This pr is for #30 . |
This pr includes the commits of last pr. I tried to solve it, but failed. So does anyone know the right way to do it? |
Sorry, I was not able to solve this problem finally. So I used force push to make commits clean. |
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.
Looks good overall! A few minor tweaks and some discussion around which eviction cause to use when popitem()
called.
src/cacheout/cache.py
Outdated
@@ -23,6 +24,23 @@ | |||
UNSET = object() | |||
|
|||
|
|||
class EvictedCause(Enum): | |||
""" | |||
An enum to represent the cause for the evication of a cache entry. |
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.
An enum to represent the cause for the evication of a cache entry. | |
An enum to represent the cause for the eviction of a cache entry. |
src/cacheout/cache.py
Outdated
@@ -23,6 +24,23 @@ | |||
UNSET = object() | |||
|
|||
|
|||
class EvictedCause(Enum): |
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.
class EvictedCause(Enum): | |
class EvictionCause(Enum): |
src/cacheout/cache.py
Outdated
@@ -513,7 +537,7 @@ def _popitem(self): | |||
raise KeyError("popitem(): cache is empty") | |||
|
|||
value = self._cache[key] | |||
self._delete(key) | |||
self._delete(key, EvictedCause.SIZE) |
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.
I wonder if _popitem()
might need the eviction cause as an argument since it could potentially be called for different reasons. It's either called from evict()
which would be SIZE but when called from popitem()
it could be considered EXPLICIT since it's similar to a direct call to delete()
except the key isn't being supplied. Seems like SIZE might not fit so well if the caller is the one making the popitem()
call (so really the cache might not be full, it's just being intentionally pruned based on the cache policy).
Just brainstorming here, but another option might be to have POLICY replace SIZE which would encompass deleting the key because either the cache was full or because the key was removed via popitem()
. But maybe wanting to differentiate between cache-full and an explicit call to remove a key would be preferable.
Thoughts?
I will add an |
After careful consideration, I don't agree |
If going the route of something like E.g.
Might be a little clearer that way, although, it is a little more implementation specific (but maybe that's fine). |
Good idea! I'll do it. |
Updated usage. Usageset cache = Cache(on_delete=on_delete)
Callable[[key: Hashable, value: Any, cause: RemovalCause], None] First parameter is the key of cache entry has been deleted. Second is the value. Third is the cause of cache entry removal.
class RemovalCause(Enum):
DELETE = auto()
SET = auto()
EXPIRED = auto()
FULL = auto()
POPITEM = auto()
TriggerThe table shows methods will trigger
⚠ |
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.
Nice work! Thanks for taking the time to add this! 👍
When are you planing to make an official release with this change? |
Released in |
An optional callback when the cache entry was deleted.