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

Client Side Caching: Alpha support #3038

Merged
merged 8 commits into from
Nov 16, 2023
Merged

Client Side Caching: Alpha support #3038

merged 8 commits into from
Nov 16, 2023

Conversation

dvora-h
Copy link
Collaborator

@dvora-h dvora-h commented Nov 13, 2023

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

Please provide a description of the change here.

@dvora-h dvora-h requested a review from chayim November 13, 2023 01:02
@chayim chayim changed the title Client Side Caching Client Side Caching: Alpha support Nov 14, 2023
redis/cache.py Outdated Show resolved Hide resolved
redis/client.py Outdated Show resolved Hide resolved
redis/client.py Outdated Show resolved Hide resolved
redis/client.py Outdated Show resolved Hide resolved
redis/client.py Outdated Show resolved Hide resolved
redis/client.py Outdated Show resolved Hide resolved
redis/utils.py Outdated Show resolved Hide resolved
redis/cache.py Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
redis/cache.py Show resolved Hide resolved
redis/cache.py Outdated Show resolved Hide resolved
Copy link
Contributor

@chayim chayim left a comment

Choose a reason for hiding this comment

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

@dvora-h see comments

@dvora-h dvora-h requested a review from chayim November 15, 2023 22:40
@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2023

Codecov Report

Attention: 63 lines in your changes are missing coverage. Please review.

Comparison is base (0113034) 91.33% compared to head (63bb591) 91.16%.

Files Patch % Lines
redis/cache.py 36.14% 53 Missing ⚠️
redis/client.py 69.69% 10 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3038      +/-   ##
==========================================
- Coverage   91.33%   91.16%   -0.18%     
==========================================
  Files         126      127       +1     
  Lines       32719    32846     +127     
==========================================
+ Hits        29884    29943      +59     
- Misses       2835     2903      +68     

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

@chayim chayim marked this pull request as ready for review November 16, 2023 07:52
@@ -597,6 +597,7 @@ async def _disconnect_raise(self, conn: Connection, error: Exception):
async def execute_command(self, *args, **options):
"""Execute a command and return a parsed response"""
await self.initialize()
options.pop("keys", None) # the keys is used only for client side caching
Copy link
Contributor

@chayim chayim Nov 16, 2023

Choose a reason for hiding this comment

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

"the keys are used only for client side caching (thoughout this PR)

@@ -0,0 +1,326 @@
import random
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep in theme we discussed, maybe this should be redis.cache.LocalCache?

_ACCESS_COUNT = "access_count"


class EvictionPolicy(Enum):
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome

Copy link
Contributor

@chayim chayim left a comment

Choose a reason for hiding this comment

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

One question, one change - change is only for comments

@dvora-h dvora-h merged commit 6978275 into redis:master Nov 16, 2023
56 checks passed
@rueian
Copy link

rueian commented Nov 16, 2023

Wow, this is cool. Will the client tracking be supported in this client cache?

@dvora-h
Copy link
Collaborator Author

dvora-h commented Nov 16, 2023

@rueian Yes, we are going to add it, this is why it's just an alpha version now I hope the next version will come soon with client tracking

@rueian
Copy link

rueian commented Nov 17, 2023

@rueian Yes, we are going to add it, this is why it's just an alpha version now I hope the next version will come soon with client tracking

Great! Can’t wait to see this powerful feature be available on redis py. This will definitely gain more acknowledgment on this feature from more developers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants