Support client side caching with ConnectionPool #3099
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3099 +/- ##
==========================================
- Coverage 91.50% 91.45% -0.06%
==========================================
Files 128 128
Lines 33053 33105 +52
==========================================
+ Hits 30245 30276 +31
- Misses 2808 2829 +21 ☔ View full report in Codecov by Sentry. |
| @@ -294,6 +294,13 @@ def __init__( | |||
| "lib_version": lib_version, | |||
| "redis_connect_func": redis_connect_func, | |||
| "protocol": protocol, | |||
| "cache_enable": cache_enable, | |||
There was a problem hiding this comment.
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?
| @@ -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.
Should we do an enforce check on RESP version, like others did - just for CSC? It might provide a better error for users.
| @@ -268,6 +267,13 @@ def __init__( | |||
| "redis_connect_func": redis_connect_func, | |||
| "credential_provider": credential_provider, | |||
| "protocol": protocol, | |||
| "cache_enable": cache_enable, | |||
There was a problem hiding this comment.
@sjpotter @vladvildanov @ofekshenawa Can you all align on these, with whatever is right collectively? Then, we can get this into the specification.
| pool = self.connection_pool | ||
| conn = self.connection or pool.get_connection(command_name, **options) | ||
| response_from_cache = conn._get_from_local_cache(args) | ||
| if response_from_cache is not None: | ||
| return response_from_cache |
There was a problem hiding this comment.
Isn't this a connection leak if we pull from cache?
We're getting a connection from the pool and never releasing it in this code path.
There was a problem hiding this comment.
Yes, it was, but fixed already in #3160
* sync * async * fixs connection mocks * fix async connection mock * fix test_asyncio/test_connection.py::test_single_connection * add test for cache blacklist and flushdb at the end of each test * fix review comments
* sync * async * fixs connection mocks * fix async connection mock * fix test_asyncio/test_connection.py::test_single_connection * add test for cache blacklist and flushdb at the end of each test * fix review comments
* sync * async * fixs connection mocks * fix async connection mock * fix test_asyncio/test_connection.py::test_single_connection * add test for cache blacklist and flushdb at the end of each test * fix review comments
This pull request extends the single mode, client-side-caching support to ConnectionPools.