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

Import some changes from redis-py #70

Merged
merged 14 commits into from
Aug 7, 2024
Merged

Import some changes from redis-py #70

merged 14 commits into from
Aug 7, 2024

Conversation

aiven-sal
Copy link
Member

@aiven-sal aiven-sal commented Aug 6, 2024

Description of change

  • Fix socket_timeout init
  • Add missing type hints for retry.py
  • Add details to the asyncio connection error message
  • Format connection errors in the same way everywhere
  • Drop unused dev-dep urllib3
  • Close Unix sockets if the connection attempt fails
  • Cluster performance improvements
  • Bump version deps
  • Fix checking of module versions
  • Avoid dangling sockets in case of SSL handshake failure
  • Get rid of event_loop warnings
  • Resolve some docs warnings
  • Ensure safe defaults for TLS

valkey/connection.py Dismissed Show dismissed Hide dismissed
tests/test_retry.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 97.89474% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.12%. Comparing base (92e3d34) to head (30cc14c).

Files Patch % Lines
tests/conftest.py 0.00% 1 Missing ⚠️
valkey/retry.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #70      +/-   ##
==========================================
+ Coverage   75.05%   75.12%   +0.06%     
==========================================
  Files         132      132              
  Lines       34378    34405      +27     
==========================================
+ Hits        25803    25847      +44     
+ Misses       8575     8558      -17     

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

@aiven-sal aiven-sal force-pushed the aiven-sal/rp_import branch from a205974 to bb8ff56 Compare August 7, 2024 10:29
@mkmkme
Copy link
Collaborator

mkmkme commented Aug 7, 2024

Yep sorry didn't know it wouldn't show you. I'm talking about commits:

dmaier-redislabs and others added 14 commits August 7, 2024 13:36
* Fixes CAE-333, which uncovered that the init method of the base
class did override the initialization of the socket_timeout parameter.

* Added missing blank lines

* Removed blank line

* Changed to quotes

---------

Co-authored-by: vladvildanov <divinez122@outlook.com>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Add missing type hints in the retry.py file and related tests.

Co-authored-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
For asyncio connection errors, include the details in the error message,
instead of only including the error code.

Co-authored-by: dmitry.kanev <dmitry.kanev@dualbootpartners.com>
Co-authored-by: Gabriel Erzse <gabriel.erzse@redis.com>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Connection errors are formatted in four places, sync and async, network
socket and unix socket. Each place has some small differences compared
to the others, while they could be, and should be, formatted in an
uniform way. Factor out the logic in a helper method and call that
method in all four places. Arguably we lose some specificity, e.g. the
words "unix socket" won't be there anymore, but it is more valuable to not
have code duplication.

Co-authored-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
This change was originally part of redis-py's d1b4191f7a

Co-authored-by: Gabriel Erzse <gabriel.erzse@redis.com>
Co-authored-by: Chayim <chayim@users.noreply.github.com>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Make sure Unix sockets get closed if the connection fails.

Co-authored-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Speed up the computation for slots when initializing a cluster. After
profiling, this turned out to be very slow, when it does not have to be.
It does not make sense to recompute the same thing over and over
in a loop.

This change was originally part of redis-py's d1b4191f7a

Co-authored-by: Gabriel Erzse <gabriel.erzse@redis.com>
Co-authored-by: Chayim <chayim@users.noreply.github.com>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
This change was originally part of redis-py's d1b4191f7a

Co-authored-by: Gabriel Erzse <gabriel.erzse@redis.com>
Co-authored-by: Chayim <chayim@users.noreply.github.com>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Not sure how it worked before, but it looks like it did not match exactly
the format in the server INFO response, i.e. MMmmPP.

This change was originally part of redis-py's d1b4191f7a

Co-authored-by: Gabriel Erzse <gabriel.erzse@redis.com>
Co-authored-by: Chayim <chayim@users.noreply.github.com>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
This change was originally part of redis-py's d1b4191f7a

Co-authored-by: Gabriel Erzse <gabriel.erzse@redis.com>
Co-authored-by: Chayim <chayim@users.noreply.github.com>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Handle more cases of failure when initializing SSL sockets, and make
sure no socket is left unclosed in case of errors.

Co-authored-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
This change was originally part of redis-py's d1b4191f7a

Co-authored-by: Gabriel Erzse <gabriel.erzse@redis.com>
Co-authored-by: Chayim <chayim@users.noreply.github.com>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Get rid of some warning related to documentation.

Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
Signed-off-by: Salvatore Mesoraca <salvatore.mesoraca@aiven.io>
@aiven-sal
Copy link
Member Author

Yep sorry didn't know it wouldn't show you. I'm talking about commits:

* [6236f51](https://github.com/valkey-io/valkey-py/commit/6236f51119ecc4a55a1b5318f6fe7871d63f41d8)

* [337a7ee](https://github.com/valkey-io/valkey-py/commit/337a7ee641a18873bfa7f00e59686a4c667b96fa)

* [1889058](https://github.com/valkey-io/valkey-py/commit/1889058211c3aa8b4f1e7016f121844a44586c34)

* [af92e15](https://github.com/valkey-io/valkey-py/commit/af92e15d69da815f2b0625bbab4be3b591ef19ca)

I added the Co-authored-by tag because those commits aren't copied verbatim. There are some minor changes in the code or the commit message compared to the original.
Of course I kept the original commit author, but I did want to unambiguously acknowledge that the code isn't exactly the one they wrote.
Maybe I'm overthinking it, but I have seen issues arising in other forks of other projects because of similar situations, with people threatening to sue etc

@aiven-sal aiven-sal force-pushed the aiven-sal/rp_import branch from bb8ff56 to 30cc14c Compare August 7, 2024 12:07
@aiven-sal aiven-sal requested a review from mkmkme August 7, 2024 12:09
@mkmkme
Copy link
Collaborator

mkmkme commented Aug 7, 2024

Yep sorry didn't know it wouldn't show you. I'm talking about commits:

* [6236f51](https://github.com/valkey-io/valkey-py/commit/6236f51119ecc4a55a1b5318f6fe7871d63f41d8)

* [337a7ee](https://github.com/valkey-io/valkey-py/commit/337a7ee641a18873bfa7f00e59686a4c667b96fa)

* [1889058](https://github.com/valkey-io/valkey-py/commit/1889058211c3aa8b4f1e7016f121844a44586c34)

* [af92e15](https://github.com/valkey-io/valkey-py/commit/af92e15d69da815f2b0625bbab4be3b591ef19ca)

I added the Co-authored-by tag because those commits aren't copied verbatim. There are some minor changes in the code or the commit message compared to the original. Of course I kept the original commit author, but I did want to unambiguously acknowledge that the code isn't exactly the one they wrote. Maybe I'm overthinking it, but I have seen issues arising in other forks of other projects because of similar situations, with people threatening to sue etc

Thanks, now I understand :) Just was wondering whether this was intentional

@aiven-sal aiven-sal merged commit 844a635 into main Aug 7, 2024
74 checks passed
@aiven-sal aiven-sal deleted the aiven-sal/rp_import branch August 7, 2024 13:44
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.

8 participants