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

Change RedisBackend to accept Redis client directly #755

Conversation

ben-cutler-datarobot
Copy link
Contributor

@ben-cutler-datarobot ben-cutler-datarobot commented Aug 18, 2023

What do these changes do?

These changes implement the feedback from this PR:
#691 (comment)
Fixes #550

This PR removes the construction of the redis.Redis object from the RedisBackend class and makes it a dependency. This allows users of the library to construct their Redis instances which might have to have a custom SSL cert, or be a more complex use case. This allows apps using this package to only have one redis instance to manage if they want to use redis for other things.

Are there changes in behavior for the user?

This PR also does a few other things (unfortunately)
0. It changes the constructor of the RedisBackend to take in a client instead of client args. This means that the RedisBackend class's public members like port will be moved to deeper in the code.

  1. it renames endpoint to host. This naming is consistent with redis and your aiomcache library
  2. It renames the redis URLstrings pool_max_size --> pool_max_size and create_connection_timeout --> create_connection_timeout to be consistent with the naming in redis-py.
  3. It removes the pool_min_size from the RedisBackend constructor (I assume I may as well remove it)

Related issue number

Fixes #691

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

aiocache/backends/redis.py Show resolved Hide resolved
aiocache/backends/redis.py Show resolved Hide resolved
aiocache/factory.py Show resolved Hide resolved
aiocache/factory.py Show resolved Hide resolved
tests/ut/backends/test_redis.py Show resolved Hide resolved
@ben-cutler-datarobot ben-cutler-datarobot mentioned this pull request Aug 18, 2023
4 tasks
tests/conftest.py Outdated Show resolved Hide resolved
@Dreamsorcerer
Copy link
Member

Looks good to me.

aiocache/factory.py Dismissed Show dismissed Hide dismissed
aiocache/factory.py Dismissed Show dismissed Hide dismissed
@ben-cutler-datarobot
Copy link
Contributor Author

@Dreamsorcerer , thank you for the feedback! LMK what you think of the changes

aiocache/factory.py Fixed Show fixed Hide fixed
@Dreamsorcerer
Copy link
Member

Seems there still some issues with the examples and the factory code. I'm wondering if it'll be easier to just create a PR for #677 first, and then update this one.

@Dreamsorcerer Dreamsorcerer added this to the 1.0 milestone Aug 22, 2023
@ben-cutler-datarobot
Copy link
Contributor Author

@Dreamsorcerer , Probably. I have a PR which fixes all the broken code in examples, let me quickly rebase and commit that

@ben-cutler-datarobot
Copy link
Contributor Author

I also can't get any of the performance tests running locally 😞 . I ran ulimit -n 12000 in hopes of fixing it, but the tests kept failing and I didn't get any errors or anything telling me anything other than the subprocess failed.

examples/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #755 (93491ab) into master (ae8405c) will increase coverage by 0.02%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #755      +/-   ##
==========================================
+ Coverage   99.73%   99.76%   +0.02%     
==========================================
  Files          35       36       +1     
  Lines        3812     3794      -18     
==========================================
- Hits         3802     3785      -17     
+ Misses         10        9       -1     
Flag Coverage Δ
unit 99.76% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
aiocache/backends/memcached.py 100.00% <100.00%> (ø)
aiocache/backends/redis.py 100.00% <100.00%> (+0.85%) ⬆️
aiocache/factory.py 100.00% <100.00%> (ø)
tests/acceptance/conftest.py 100.00% <100.00%> (ø)
tests/acceptance/test_factory.py 100.00% <100.00%> (ø)
tests/conftest.py 100.00% <100.00%> (ø)
tests/performance/conftest.py 100.00% <100.00%> (ø)
tests/performance/server.py 100.00% <100.00%> (ø)
tests/ut/backends/test_memcached.py 100.00% <100.00%> (ø)
tests/ut/backends/test_redis.py 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae8405c...93491ab. Read the comment docs.

@Dreamsorcerer Dreamsorcerer changed the title [550] Isolate client attribute from RedisBackend class Change RedisBackend to accept Redis client directly Aug 22, 2023
@Dreamsorcerer
Copy link
Member

Great, thanks for getting all of that working!

@Dreamsorcerer Dreamsorcerer merged commit f83f43b into aio-libs:master Aug 22, 2023
14 checks passed
@ben-cutler-datarobot
Copy link
Contributor Author

@Dreamsorcerer , Thanks for helping get this over the finish line, and building this library!

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

Successfully merging this pull request may close these issues.

SSL/TLS support for redis cache
2 participants