-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Streamline client side caching API typing #3216
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
Changes from 1 commit
523dc7c
93709ea
a157347
106d794
48b43f4
0d60ff4
4691a9c
eab6973
fcebcaa
3a9d360
27c5544
92608b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
click==8.0.4 | ||
black==24.3.0 | ||
cachetools | ||
flake8==5.0.4 | ||
flake8-isort==6.0.0 | ||
flynt~=0.69.0 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
sentinel monitor redis-py-test 127.0.0.1 6379 2 | ||
sentinel resolve-hostnames yes | ||
sentinel monitor redis-py-test redis 6379 2 | ||
sentinel down-after-milliseconds redis-py-test 5000 | ||
sentinel failover-timeout redis-py-test 60000 | ||
sentinel parallel-syncs redis-py-test 1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,13 +4,12 @@ | |
from abc import ABC, abstractmethod | ||
from collections import OrderedDict, defaultdict | ||
from enum import Enum | ||
from typing import List | ||
from typing import Iterable, List, Union | ||
|
||
from redis.typing import KeyT, ResponseT | ||
|
||
DEFAULT_EVICTION_POLICY = "lru" | ||
|
||
|
||
DEFAULT_BLACKLIST = [ | ||
"BF.CARD", | ||
"BF.DEBUG", | ||
|
@@ -71,7 +70,6 @@ | |
"TTL", | ||
] | ||
|
||
|
||
DEFAULT_WHITELIST = [ | ||
"BITCOUNT", | ||
"BITFIELD_RO", | ||
|
@@ -180,7 +178,7 @@ def delete_command(self, command: str): | |
pass | ||
|
||
@abstractmethod | ||
def delete_many(self, commands): | ||
def delete_commands(self, commands): | ||
pass | ||
|
||
@abstractmethod | ||
|
@@ -215,7 +213,6 @@ def __init__( | |
max_size: int = 10000, | ||
ttl: int = 0, | ||
eviction_policy: EvictionPolicy = DEFAULT_EVICTION_POLICY, | ||
**kwargs, | ||
): | ||
self.max_size = max_size | ||
self.ttl = ttl | ||
|
@@ -224,12 +221,17 @@ def __init__( | |
self.key_commands_map = defaultdict(set) | ||
self.commands_ttl_list = [] | ||
|
||
def set(self, command: str, response: ResponseT, keys_in_command: List[KeyT]): | ||
def set( | ||
self, | ||
command: Union[str, Iterable[str]], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why it need to be also Iterable[str]? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because in all unit tests tuples are being sent for this parameter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not in unit tests, that is for the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So maybe we need only Iterable[str]? I'm not sure... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a broader discussion, I think we can leave it with both for the beta release. In general I think we should not expose to the outside world the way we represent commands in the cache. We should only expose the concept of keys externally, and do the mapping to commands and back only internally, and then we have full freedom of representation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used |
||
response: ResponseT, | ||
keys_in_command: List[KeyT], | ||
): | ||
""" | ||
Set a redis command and its response in the cache. | ||
|
||
Args: | ||
command (str): The redis command. | ||
command (Union[str, Iterable[str]]): The redis command. | ||
response (ResponseT): The response associated with the command. | ||
keys_in_command (List[KeyT]): The list of keys used in the command. | ||
""" | ||
|
@@ -244,12 +246,12 @@ def set(self, command: str, response: ResponseT, keys_in_command: List[KeyT]): | |
self._update_key_commands_map(keys_in_command, command) | ||
self.commands_ttl_list.append(command) | ||
|
||
def get(self, command: str) -> ResponseT: | ||
def get(self, command: Union[str, Iterable[str]]) -> ResponseT: | ||
""" | ||
Get the response for a redis command from the cache. | ||
|
||
Args: | ||
command (str): The redis command. | ||
command (Union[str, Iterable[str]]): The redis command. | ||
|
||
Returns: | ||
ResponseT: The response associated with the command, or None if the command is not in the cache. # noqa | ||
|
@@ -261,34 +263,42 @@ def get(self, command: str) -> ResponseT: | |
self._update_access(command) | ||
return copy.deepcopy(self.cache[command]["response"]) | ||
|
||
def delete_command(self, command: str): | ||
def delete_command(self, command: Union[str, Iterable[str]]): | ||
""" | ||
Delete a redis command and its metadata from the cache. | ||
|
||
Args: | ||
command (str): The redis command to be deleted. | ||
command (Union[str, Iterable[str]]): The redis command to be deleted. | ||
""" | ||
if command in self.cache: | ||
keys_in_command = self.cache[command].get("keys") | ||
self._del_key_commands_map(keys_in_command, command) | ||
self.commands_ttl_list.remove(command) | ||
del self.cache[command] | ||
|
||
def delete_many(self, commands): | ||
pass | ||
def delete_commands(self, commands: List[Union[str, Iterable[str]]]): | ||
""" | ||
Delete multiple commands and their metadata from the cache. | ||
|
||
Args: | ||
commands (List[Union[str, Iterable[str]]]): The list of commands to be | ||
deleted. | ||
""" | ||
for command in commands: | ||
self.delete_command(command) | ||
|
||
def flush(self): | ||
"""Clear the entire cache, removing all redis commands and metadata.""" | ||
self.cache.clear() | ||
self.key_commands_map.clear() | ||
self.commands_ttl_list = [] | ||
|
||
def _is_expired(self, command: str) -> bool: | ||
def _is_expired(self, command: Union[str, Iterable[str]]) -> bool: | ||
""" | ||
Check if a redis command has expired based on its time-to-live. | ||
|
||
Args: | ||
command (str): The redis command. | ||
command (Union[str, Iterable[str]]): The redis command. | ||
|
||
Returns: | ||
bool: True if the command has expired, False otherwise. | ||
|
@@ -297,12 +307,12 @@ def _is_expired(self, command: str) -> bool: | |
return False | ||
return time.monotonic() - self.cache[command]["ctime"] > self.ttl | ||
|
||
def _update_access(self, command: str): | ||
def _update_access(self, command: Union[str, Iterable[str]]): | ||
""" | ||
Update the access information for a redis command based on the eviction policy. | ||
|
||
Args: | ||
command (str): The redis command. | ||
command (Union[str, Iterable[str]]): The redis command. | ||
""" | ||
if self.eviction_policy == EvictionPolicy.LRU.value: | ||
self.cache.move_to_end(command) | ||
|
@@ -329,24 +339,28 @@ def _evict(self): | |
random_command = random.choice(list(self.cache.keys())) | ||
self.cache.pop(random_command) | ||
|
||
def _update_key_commands_map(self, keys: List[KeyT], command: str): | ||
def _update_key_commands_map( | ||
self, keys: List[KeyT], command: Union[str, Iterable[str]] | ||
): | ||
""" | ||
Update the key_commands_map with command that uses the keys. | ||
|
||
Args: | ||
keys (List[KeyT]): The list of keys used in the command. | ||
command (str): The redis command. | ||
command (Union[str, Iterable[str]]): The redis command. | ||
""" | ||
for key in keys: | ||
self.key_commands_map[key].add(command) | ||
|
||
def _del_key_commands_map(self, keys: List[KeyT], command: str): | ||
def _del_key_commands_map( | ||
self, keys: List[KeyT], command: Union[str, Iterable[str]] | ||
): | ||
""" | ||
Remove a redis command from the key_commands_map. | ||
|
||
Args: | ||
keys (List[KeyT]): The list of keys used in the redis command. | ||
command (str): The redis command. | ||
command (Union[str, Iterable[str]]): The redis command. | ||
""" | ||
for key in keys: | ||
self.key_commands_map[key].remove(command) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,11 @@ async def r(request, create_redis): | |
yield r, cache | ||
|
||
|
||
@pytest_asyncio.fixture() | ||
async def local_cache(): | ||
yield _LocalCache() | ||
|
||
|
||
@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only") | ||
class TestLocalCache: | ||
@pytest.mark.onlynoncluster | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? I think it will fail on cluster There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved the decorator to class level. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved back. |
||
|
@@ -228,3 +233,43 @@ async def test_cache_decode_response(self, r): | |
assert cache.get(("GET", "foo")) is None | ||
# get key from redis | ||
assert await r.get("foo") == "barbar" | ||
|
||
|
||
@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only") | ||
@pytest.mark.onlynoncluster | ||
class TestSentinelLocalCache: | ||
|
||
async def test_get_from_cache(self, local_cache, master): | ||
await master.set("foo", "bar") | ||
# get key from redis and save in local cache | ||
assert await master.get("foo") == b"bar" | ||
# get key from local cache | ||
assert local_cache.get(("GET", "foo")) == b"bar" | ||
# change key in redis (cause invalidation) | ||
await master.set("foo", "barbar") | ||
# send any command to redis (process invalidation in background) | ||
await master.ping() | ||
vladvildanov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# the command is not in the local cache anymore | ||
assert local_cache.get(("GET", "foo")) is None | ||
# get key from redis | ||
assert await master.get("foo") == b"barbar" | ||
|
||
@pytest.mark.parametrize( | ||
"sentinel_setup", | ||
[{"kwargs": {"decode_responses": True}}], | ||
indirect=True, | ||
) | ||
async def test_cache_decode_response(self, local_cache, sentinel_setup, master): | ||
await master.set("foo", "bar") | ||
# get key from redis and save in local cache | ||
assert await master.get("foo") == "bar" | ||
# get key from local cache | ||
assert local_cache.get(("GET", "foo")) == "bar" | ||
# change key in redis (cause invalidation) | ||
await master.set("foo", "barbar") | ||
# send any command to redis (process invalidation in background) | ||
await master.ping() | ||
# the command is not in the local cache anymore | ||
assert local_cache.get(("GET", "foo")) is None | ||
# get key from redis | ||
assert await master.get("foo") == "barbar" |
Uh oh!
There was an error while loading. Please reload this page.