-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Support client side caching with ConnectionPool #3099
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
46f918d
sync
dvora-h be39f47
async
dvora-h 583ac94
fixs connection mocks
dvora-h a2317ab
fix async connection mock
dvora-h cea3458
fix test_asyncio/test_connection.py::test_single_connection
dvora-h 29f60c4
add test for cache blacklist and flushdb at the end of each test
dvora-h 1b2387b
fix review comments
dvora-h File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,9 +47,15 @@ | |
ResponseError, | ||
TimeoutError, | ||
) | ||
from redis.typing import EncodableT | ||
from redis.typing import EncodableT, KeysT, ResponseT | ||
from redis.utils import HIREDIS_AVAILABLE, get_lib_version, str_if_bytes | ||
|
||
from .._cache import ( | ||
DEFAULT_BLACKLIST, | ||
DEFAULT_EVICTION_POLICY, | ||
DEFAULT_WHITELIST, | ||
_LocalCache, | ||
) | ||
from .._parsers import ( | ||
BaseParser, | ||
Encoder, | ||
|
@@ -114,6 +120,9 @@ class AbstractConnection: | |
"encoder", | ||
"ssl_context", | ||
"protocol", | ||
"client_cache", | ||
"cache_blacklist", | ||
"cache_whitelist", | ||
"_reader", | ||
"_writer", | ||
"_parser", | ||
|
@@ -148,6 +157,13 @@ def __init__( | |
encoder_class: Type[Encoder] = Encoder, | ||
credential_provider: Optional[CredentialProvider] = None, | ||
protocol: Optional[int] = 2, | ||
cache_enable: bool = False, | ||
client_cache: Optional[_LocalCache] = None, | ||
cache_max_size: int = 100, | ||
cache_ttl: int = 0, | ||
cache_eviction_policy: str = DEFAULT_EVICTION_POLICY, | ||
cache_blacklist: List[str] = DEFAULT_BLACKLIST, | ||
cache_whitelist: List[str] = DEFAULT_WHITELIST, | ||
): | ||
if (username or password) and credential_provider is not None: | ||
raise DataError( | ||
|
@@ -205,6 +221,14 @@ def __init__( | |
if p < 2 or p > 3: | ||
raise ConnectionError("protocol must be either 2 or 3") | ||
self.protocol = protocol | ||
if cache_enable: | ||
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. Should we do an enforce check on RESP version, like others did - just for CSC? It might provide a better error for users. |
||
_cache = _LocalCache(cache_max_size, cache_ttl, cache_eviction_policy) | ||
else: | ||
_cache = None | ||
self.client_cache = client_cache if client_cache is not None else _cache | ||
if self.client_cache is not None: | ||
self.cache_blacklist = cache_blacklist | ||
self.cache_whitelist = cache_whitelist | ||
|
||
def __del__(self, _warnings: Any = warnings): | ||
# For some reason, the individual streams don't get properly garbage | ||
|
@@ -395,6 +419,11 @@ async def on_connect(self) -> None: | |
# if a database is specified, switch to it. Also pipeline this | ||
if self.db: | ||
await self.send_command("SELECT", self.db) | ||
# if client caching is enabled, start tracking | ||
if self.client_cache: | ||
await self.send_command("CLIENT", "TRACKING", "ON") | ||
await self.read_response() | ||
self._parser.set_invalidation_push_handler(self._cache_invalidation_process) | ||
|
||
# read responses from pipeline | ||
for _ in (sent for sent in (self.lib_name, self.lib_version) if sent): | ||
|
@@ -429,6 +458,9 @@ async def disconnect(self, nowait: bool = False) -> None: | |
raise TimeoutError( | ||
f"Timed out closing connection after {self.socket_connect_timeout}" | ||
) from None | ||
finally: | ||
if self.client_cache: | ||
dvora-h marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.client_cache.flush() | ||
|
||
async def _send_ping(self): | ||
"""Send PING, expect PONG in return""" | ||
|
@@ -646,10 +678,62 @@ def pack_commands(self, commands: Iterable[Iterable[EncodableT]]) -> List[bytes] | |
output.append(SYM_EMPTY.join(pieces)) | ||
return output | ||
|
||
def _is_socket_empty(self): | ||
def _socket_is_empty(self): | ||
"""Check if the socket is empty""" | ||
return not self._reader.at_eof() | ||
|
||
def _cache_invalidation_process( | ||
self, data: List[Union[str, Optional[List[str]]]] | ||
) -> None: | ||
""" | ||
Invalidate (delete) all redis commands associated with a specific key. | ||
`data` is a list of strings, where the first string is the invalidation message | ||
and the second string is the list of keys to invalidate. | ||
(if the list of keys is None, then all keys are invalidated) | ||
""" | ||
if data[1] is not None: | ||
dvora-h marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.client_cache.flush() | ||
else: | ||
for key in data[1]: | ||
self.client_cache.invalidate(str_if_bytes(key)) | ||
|
||
async def _get_from_local_cache(self, command: str): | ||
""" | ||
If the command is in the local cache, return the response | ||
""" | ||
if ( | ||
self.client_cache is None | ||
or command[0] in self.cache_blacklist | ||
or command[0] not in self.cache_whitelist | ||
): | ||
return None | ||
while not self._socket_is_empty(): | ||
await self.read_response(push_request=True) | ||
return self.client_cache.get(command) | ||
|
||
def _add_to_local_cache( | ||
self, command: Tuple[str], response: ResponseT, keys: List[KeysT] | ||
): | ||
""" | ||
Add the command and response to the local cache if the command | ||
is allowed to be cached | ||
""" | ||
if ( | ||
self.client_cache is not None | ||
and (self.cache_blacklist == [] or command[0] not in self.cache_blacklist) | ||
and (self.cache_whitelist == [] or command[0] in self.cache_whitelist) | ||
): | ||
self.client_cache.set(command, response, keys) | ||
|
||
def delete_from_local_cache(self, command: str): | ||
""" | ||
Delete the command from the local cache | ||
""" | ||
try: | ||
self.client_cache.delete(command) | ||
except AttributeError: | ||
pass | ||
|
||
|
||
class Connection(AbstractConnection): | ||
"Manages TCP communication to and from a Redis server" | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Let's have you, @vladvildanov and @uglide sync on these just to finalize before the next release. Getting them into the specs finally too - WDYT?