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

Streamline client side caching API typing #3216

Merged
merged 12 commits into from
May 8, 2024

Conversation

gerzse
Copy link
Contributor

@gerzse gerzse commented Apr 25, 2024

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Streamline the typing of the client side caching API. Some of the methods are defining commands of type str, while in reality tuples are being sent for those parameters.

Add client side cache tests for Sentinels. In order to make this work, fix the sentinel configuration in the docker-compose stack.

Add a test for client side caching with a truly custom cache, not just injecting our internal cache structure as custom.

Add a test for client side caching where two different types of commands use the same key, to make sure they invalidate each others cached data.

@gerzse gerzse requested a review from uglide April 25, 2024 12:25
Copy link

codecov bot commented Apr 25, 2024

Codecov Report

Attention: Patch coverage is 98.57143% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 91.94%. Comparing base (2f88840) to head (523dc7c).
Report is 20 commits behind head on master.

Files Patch % Lines
tests/test_cache.py 97.87% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3216      +/-   ##
==========================================
+ Coverage   91.84%   91.94%   +0.09%     
==========================================
  Files         128      128              
  Lines       33232    33517     +285     
==========================================
+ Hits        30523    30816     +293     
+ Misses       2709     2701       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gerzse gerzse force-pushed the client-side-caching-cleanup branch 2 times, most recently from 0436538 to a55cdb1 Compare April 25, 2024 13:16
Streamline the typing of the client side caching API. Some of the
methods are defining commands of type `str`, while in reality tuples
are being sent for those parameters.

Add client side cache tests for Sentinels. In order to make this work,
fix the sentinel configuration in the docker-compose stack.

Add a test for client side caching with a truly custom cache, not just
injecting our internal cache structure as custom.

Add a test for client side caching where two different types of commands
use the same key, to make sure they invalidate each others cached data.
@gerzse gerzse requested a review from dvora-h April 30, 2024 10:29
@@ -696,7 +696,7 @@ def _cache_invalidation_process(
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:
if data[1] is None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surfaced due to the new tests, basically luck.

@pytest.mark.skipif(HIREDIS_AVAILABLE, reason="PythonParser only")
class TestLocalCache:
@pytest.mark.onlynoncluster
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? I think it will fail on cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the decorator to class level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved back.

@@ -36,7 +41,7 @@ async def test_get_from_cache(self, r, r2):
assert await r.get("foo") == b"barbar"

@pytest.mark.parametrize("r", [{"cache": _LocalCache(max_size=3)}], indirect=True)
async def test_cache_max_size(self, r):
async def test_cache_lru_eviction(self, r):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? this test checks that if I set the max_size to 3 it will not save more than 3 in the cache

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a discussion about eviction policies. Max size is always bound to an eviction policy. It felt cleaner to express this as "test that the default eviction policy is LRU".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you refer me to this discussion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The discussion was mostly online, in the weekly call. Another argument: all tests for eviction strategies implicitly test the max size of the cache.

tests/test_asyncio/test_cache.py Show resolved Hide resolved
dev_requirements.txt Show resolved Hide resolved
redis/_cache.py Outdated
def set(self, command: str, response: ResponseT, keys_in_command: List[KeyT]):
def set(
self,
command: Union[str, Iterable[str]],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it need to be also Iterable[str]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in all unit tests tuples are being sent for this parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in unit tests, that is for the get. In Connection we have this signature for example:

def _add_to_local_cache(
    self, command: Tuple[str], response: ResponseT, keys: List[KeysT]
):

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe we need only Iterable[str]? I'm not sure...

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used Sequence instead of Iterable and Tuple. I think it is the most expressive one.

tests/test_cache.py Outdated Show resolved Hide resolved
[{"cache": _LocalCache(max_size=3)}],
indirect=True,
)
def test_cache_lru_eviction(self, r):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as in the async test (naming)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, same reason as above.

@@ -730,6 +730,27 @@ def _add_to_local_cache(
):
self.client_cache.set(command, response, keys)

def flush_cache(self):
try:
if self.client_cache:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need the if inside the try block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try is meant to compensate for the if? You mean we could leave the if without the try? I would find that cleaner.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if-statement will prevent the call to flush/delete_command/invalidate_key that would cause the AttributeError if client_cache is falsy. The try-catch-block will also catch an AttributeError if client_cache is truthy but doesn't provide the method.
I'm missing to much context to say which is more desirable. But if it can be assumed, that a thruthy client_cache always also provides the methods, then the if-statement and try-catch are redundant and it becomes a question of style. You can read more on that here https://realpython.com/python-lbyl-vs-eafp/
Best regards, a random passerby XD

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CBerndt-Work for your nice explanation. The article is very well written. I realized that I am by now too conditioned towards LBYL, from other languages. Especially here, using AttributeError, which signals issues with the code structure, not with data. So I went for the ifs.

@dvora-h dvora-h added bug Bug maintenance Maintenance (CI, Releases, etc) labels May 8, 2024
@dvora-h dvora-h merged commit 3bd311c into redis:master May 8, 2024
47 checks passed
vladvildanov pushed a commit that referenced this pull request Sep 27, 2024
* Streamline client side caching API typing

Streamline the typing of the client side caching API. Some of the
methods are defining commands of type `str`, while in reality tuples
are being sent for those parameters.

Add client side cache tests for Sentinels. In order to make this work,
fix the sentinel configuration in the docker-compose stack.

Add a test for client side caching with a truly custom cache, not just
injecting our internal cache structure as custom.

Add a test for client side caching where two different types of commands
use the same key, to make sure they invalidate each others cached data.

* Fixes after running tests against RE

* More test cases

* Fix async tests

* Tests for raw commands

* Change terminology for allow/deny lists

* Add test for single connection

* Make sure flushing the cache works everywhere

* Reenable some tests for cluster too

* Align cache typings at abstract level

* Use Sequence instead of Iterable for types

* Remove some exceptions in favor of ifs

---------

Co-authored-by: Gabriel Erzse <gabriel.erzse@redis.com>
vladvildanov pushed a commit that referenced this pull request Sep 27, 2024
* Streamline client side caching API typing

Streamline the typing of the client side caching API. Some of the
methods are defining commands of type `str`, while in reality tuples
are being sent for those parameters.

Add client side cache tests for Sentinels. In order to make this work,
fix the sentinel configuration in the docker-compose stack.

Add a test for client side caching with a truly custom cache, not just
injecting our internal cache structure as custom.

Add a test for client side caching where two different types of commands
use the same key, to make sure they invalidate each others cached data.

* Fixes after running tests against RE

* More test cases

* Fix async tests

* Tests for raw commands

* Change terminology for allow/deny lists

* Add test for single connection

* Make sure flushing the cache works everywhere

* Reenable some tests for cluster too

* Align cache typings at abstract level

* Use Sequence instead of Iterable for types

* Remove some exceptions in favor of ifs

---------

Co-authored-by: Gabriel Erzse <gabriel.erzse@redis.com>
vladvildanov pushed a commit that referenced this pull request Sep 27, 2024
* Streamline client side caching API typing

Streamline the typing of the client side caching API. Some of the
methods are defining commands of type `str`, while in reality tuples
are being sent for those parameters.

Add client side cache tests for Sentinels. In order to make this work,
fix the sentinel configuration in the docker-compose stack.

Add a test for client side caching with a truly custom cache, not just
injecting our internal cache structure as custom.

Add a test for client side caching where two different types of commands
use the same key, to make sure they invalidate each others cached data.

* Fixes after running tests against RE

* More test cases

* Fix async tests

* Tests for raw commands

* Change terminology for allow/deny lists

* Add test for single connection

* Make sure flushing the cache works everywhere

* Reenable some tests for cluster too

* Align cache typings at abstract level

* Use Sequence instead of Iterable for types

* Remove some exceptions in favor of ifs

---------

Co-authored-by: Gabriel Erzse <gabriel.erzse@redis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug maintenance Maintenance (CI, Releases, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants