async_cluster: fix max_connections/ssl & improve args #2217

utkarshgupta137 · 2022-06-05T21:27:23Z

Pull Request check-list

Does $ tox pass with this change (including linting)?
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?

Description of change

set proper connection_class if ssl = True
pass max_connections/connection_class to ClusterNode
pass parser_class to Connection instead of changing it in on_connect
only pass redis_connect_func if read_from_replicas = True
add connection_error_retry_attempts parameter
skip is_connected check in acquire_connection as it is already checked in send_packed_command

BREAKING:

RedisCluster args except host & port are kw-only now
RedisCluster will no longer accept unknown arguments
RedisCluster will no longer accept url as an argument. Use RedisCluster.from_url
RedisCluster.require_full_coverage defaults to True
ClusterNode args except host, port, & server_type are kw-only now

codecov-commenter · 2022-06-05T21:35:02Z

Codecov Report

Merging #2217 (79de404) into master (a246f40) will increase coverage by 0.01%.
The diff coverage is 94.03%.

@@            Coverage Diff             @@
##           master    #2217      +/-   ##
==========================================
+ Coverage   92.00%   92.01%   +0.01%     
==========================================
  Files         109      109              
  Lines       28068    28133      +65     
==========================================
+ Hits        25823    25887      +64     
- Misses       2245     2246       +1

Impacted Files	Coverage Δ
redis/asyncio/cluster.py	`90.28% <91.52%> (+0.35%)`	⬆️
tests/test_asyncio/test_cluster.py	`96.95% <95.45%> (-0.49%)`	⬇️
redis/cluster.py	`90.37% <100.00%> (ø)`
redis/exceptions.py	`98.55% <100.00%> (+0.04%)`	⬆️
tests/test_cluster.py	`96.85% <0.00%> (-0.24%)`	⬇️
tests/test_graph.py	`91.53% <0.00%> (-0.03%)`	⬇️
redis/asyncio/connection.py	`85.24% <0.00%> (+1.10%)`	⬆️

Continue to review full report at Codecov.

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

dvora-h · 2022-06-23T12:12:02Z

redis/asyncio/cluster.py

@@ -234,87 +208,132 @@ def from_url(cls, url: str, **kwargs: Any) -> "RedisCluster":
    def __init__(
        self,
        host: Optional[str] = None,
-        port: int = 6379,
+        port: Union[str, int] = 6379,
+        *,


@utkarshgupta137 Why do you think it's better to force send arguments as kw only?

This is the same approach followed in the simple async client: https://github.com/redis/redis-py/blob/master/redis/asyncio/client.py#L144

In case we add/remove kwargs, it becomes simpler for users to change the required kwargs.

This starts us down a path split between the two connection objects.
Having that conversation is reasonable, but IMHO not in the 4.x series. It should be part of a general architectural change (i.e maybe sansio and friends).

There is already a split between the client objects. For eg., Added dynamic_startup_nodes configuration to RedisCluster. #2244 was recently merged which added a new kwarg to the sync client but not to the async client.

Now, in case the user expects both clients to mirror each other & tries setting it as a positional argument, then they may receive an improper error or worse they're actually changing some other parameter they don't indent to.

This is also the reason that I've removed support for unknown kwargs so that the user knows when an unsupported kwarg is passed instead of it silently getting ignored.

The host/port/startup_nodes are currently the first 3 arguments. Most users will either use the first 2 or the 3rd, but not both. So most users would already be passing the rest of the arguments as kwargs, making it a minor difference between the two.

Still, if preferred, I can remove this restriction.

Realistically, I think the issue is that we don't have the same arguments for all (supported) connection types where possible. I'm okay with adding arguments to sync these, as long as we don't change ordering.

Basically - no backwards compatibility issues.

Yes, but how would it work in the case of this new kwarg? Are you suggesting that all new options should always be added to the async client or are you suggesting that we add dummy kwargs to keep the clients in sync? Because the latter sounds very counter-intuitive from the user's point of view.

I think the two clients actually should probably inherit from a common base, then work it through there...

This is actually how it works in the rust client. Although, in their case, the base class is the client from which you get sync or async connections. I think that is a better experience from a user perspective, but it would be a breaking change and probably needs to be discussed in #2119.

utkarshgupta137 · 2022-07-06T05:17:37Z

@chayim @dvora-h Any updates on this?

utkarshgupta137 · 2022-07-24T12:44:31Z

@chayim @dvora-h Would dropping the kw-only required be enough to get this merged for now? I'm still strongly in favour of it, but this PR contains other important fixes which should be merged while we continue to have that discussion.

chayim · 2022-07-24T12:53:11Z

I think this is fine. @dvora-h want to do another run through to make sure there isn't an argument that's inadvertently removed (added is fine). I think this belongs in 4.4.x

utkarshgupta137 · 2022-07-24T12:58:09Z

Ok, in that case, I'll just rebase this PR.

- set proper connection_class if ssl = True - pass max_connections/connection_class to ClusterNode - recreate startup_nodes to properly initialize - pass parser_class to Connection instead of changing it in on_connect - only pass redis_connect_func if read_from_replicas = True - add connection_error_retry_attempts parameter - skip is_connected check in acquire_connection as it is already checked in send_packed_command BREAKING: - RedisCluster args except host & port are kw-only now - RedisCluster will no longer accept unknown arguments - RedisCluster will no longer accept url as an argument. Use RedisCluster.from_url - RedisCluster.require_full_coverage defaults to True - ClusterNode args except host, port, & server_type are kw-only now

dvora-h · 2022-07-24T13:10:51Z

@utkarshgupta137 @chayim I think that except the kwargs issue - this can be merged

* async_cluster: fix max_connections/ssl & improve args - set proper connection_class if ssl = True - pass max_connections/connection_class to ClusterNode - recreate startup_nodes to properly initialize - pass parser_class to Connection instead of changing it in on_connect - only pass redis_connect_func if read_from_replicas = True - add connection_error_retry_attempts parameter - skip is_connected check in acquire_connection as it is already checked in send_packed_command BREAKING: - RedisCluster args except host & port are kw-only now - RedisCluster will no longer accept unknown arguments - RedisCluster will no longer accept url as an argument. Use RedisCluster.from_url - RedisCluster.require_full_coverage defaults to True - ClusterNode args except host, port, & server_type are kw-only now * async_cluster: remove kw-only requirement from client Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com>

* Add support for async graph * linters * fix docstring * Use retry mechanism in async version of Connection objects (#2271) * fix is_connected (#2278) * fix: workaround asyncio bug on connection reset by peer (#2259) Fixes #2237 * Fix crash: key expire while search (#2270) * fix expire while search * sleep * docs: Fix a few typos (#2274) * docs: Fix a few typos There are small typos in: - redis/cluster.py - redis/commands/core.py - redis/ocsp.py - tests/test_cluster.py Fixes: - Should read `validity` rather than `valididy`. - Should read `reinitialize` rather than `reinitilize`. - Should read `farthest` rather than `farest`. - Should read `commands` rather than `comamnds`. * Update core.py * async_cluster: fix concurrent pipeline (#2280) - each pipeline should create separate stacks for each node * Add support for TIMESERIES 1.8 (#2296) * Add support for timeseries 1.8 * fix info * linters * linters * fix info test * type hints * linters * Remove verbose logging from `redis-py/redis/cluster.py` (#2238) * removed the logging module and its corresponding methods * updated CHANGES * except block for RedisClusterException and BusyLoadingError removed * removed unused import (redis.exceptions.BusyLoadingError) * empty commit to re-trigger Actions workflow * replaced BaseException with Exception * empty commit to re-trigger Actions workflow * empty commit to re-trigger Actions workflow * redundant logic removed * re-trigger pipeline * reverted changes * re-trigger pipeline * except logic changed * redis stream example (#2269) * redis stream example * redis stream example on docs/examples.rst Co-authored-by: pedro.frazao <perl.pf@netcf.org> * Fix: `start_id` type for `XAUTOCLAIM` (#2257) * Changed start_id type for xautoclaim * Added to changes Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com> * Doc add timeseries example (#2267) * DOC add timeseries example * DOC add timeseries examples * Apply suggestions * Fix typo Detention period => Retention period Co-authored-by: Gauthier Imbert <gauthier@PC17> * Fix warnings and resource usage problems in asyncio unittests (#2258) * Use pytest-asyncio in auto mode Remove overly genereric `pytestmark=pytest.mark.asyncio` causing lots of warning noise * Use "Factories as Fixtures" test pattern for the `create_redis` fixture this fixture is now async, avoiding teardown problems with missing event loops. * Fix sporadic error on fast event loops, such as `--uvloop` * Close connection, even if "username" was in kwargs This fixes a resource usage warning in the async unittests. * Do async cleanup of acl passwords via a fixture * Remove unused import, fix whitespace * Fix test with missing "await" * Close pubsub objects after use in unittest Use a simple fixture where possible, otherwise manually call pubsub.close() * re-introduce `pytestmark=pytest.mark.asyncio` for python 3.6 * Use context manager to clean up connections in connection pool for unit tests * Provide asynccontextmanager for python 3.6 * make `test_late_subscribe()` more robuste * Catch a couple of additional leaked resources * Graph - add counters for removed labels and properties (#2292) * grpah - add counters for removed labels and properties * added mock graph result set statistics * docstrings for graph result set statistics * format * isort * moved docstrings into functions * cleaning up the readme and moving docs into readthedocs (#2291) * cleaning up the readme and moving docs into readthedocs * examples at the end as per pr comments * async_cluster: fix max_connections/ssl & improve args (#2217) * async_cluster: fix max_connections/ssl & improve args - set proper connection_class if ssl = True - pass max_connections/connection_class to ClusterNode - recreate startup_nodes to properly initialize - pass parser_class to Connection instead of changing it in on_connect - only pass redis_connect_func if read_from_replicas = True - add connection_error_retry_attempts parameter - skip is_connected check in acquire_connection as it is already checked in send_packed_command BREAKING: - RedisCluster args except host & port are kw-only now - RedisCluster will no longer accept unknown arguments - RedisCluster will no longer accept url as an argument. Use RedisCluster.from_url - RedisCluster.require_full_coverage defaults to True - ClusterNode args except host, port, & server_type are kw-only now * async_cluster: remove kw-only requirement from client Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com> * fix review comments * fix * fix review comments * fix review comments Co-authored-by: Chayim <chayim@users.noreply.github.com> Co-authored-by: szumka <106675199+szumka@users.noreply.github.com> Co-authored-by: Mehdi ABAAKOUK <sileht@sileht.net> Co-authored-by: Tim Gates <tim.gates@iress.com> Co-authored-by: Utkarsh Gupta <utkarshgupta137@gmail.com> Co-authored-by: Nial Daly <34862917+nialdaly@users.noreply.github.com> Co-authored-by: pedrofrazao <603718+pedrofrazao@users.noreply.github.com> Co-authored-by: pedro.frazao <perl.pf@netcf.org> Co-authored-by: Антон Безденежных <gamer392@yandex.ru> Co-authored-by: Iglesys <g.imbert34@gmail.com> Co-authored-by: Gauthier Imbert <gauthier@PC17> Co-authored-by: Kristján Valur Jónsson <sweskman@gmail.com> Co-authored-by: DvirDukhan <dvir@redis.com>

utkarshgupta137 · 2022-09-08T23:27:39Z

@chayim @dvora-h It has been 3 months since this PR was raised & 2 months since it was merged. This is a major bug fix which should've been released instantly. Can we please create a minor release with this and other async cluster related bug fixes?

* async_cluster: fix max_connections/ssl & improve args - set proper connection_class if ssl = True - pass max_connections/connection_class to ClusterNode - recreate startup_nodes to properly initialize - pass parser_class to Connection instead of changing it in on_connect - only pass redis_connect_func if read_from_replicas = True - add connection_error_retry_attempts parameter - skip is_connected check in acquire_connection as it is already checked in send_packed_command BREAKING: - RedisCluster args except host & port are kw-only now - RedisCluster will no longer accept unknown arguments - RedisCluster will no longer accept url as an argument. Use RedisCluster.from_url - RedisCluster.require_full_coverage defaults to True - ClusterNode args except host, port, & server_type are kw-only now * async_cluster: remove kw-only requirement from client Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com>

* fix is_connected (#2278) * fix: workaround asyncio bug on connection reset by peer (#2259) Fixes #2237 * Fix crash: key expire while search (#2270) * fix expire while search * sleep * async_cluster: fix concurrent pipeline (#2280) - each pipeline should create separate stacks for each node * async_cluster: fix max_connections/ssl & improve args (#2217) * async_cluster: fix max_connections/ssl & improve args - set proper connection_class if ssl = True - pass max_connections/connection_class to ClusterNode - recreate startup_nodes to properly initialize - pass parser_class to Connection instead of changing it in on_connect - only pass redis_connect_func if read_from_replicas = True - add connection_error_retry_attempts parameter - skip is_connected check in acquire_connection as it is already checked in send_packed_command BREAKING: - RedisCluster args except host & port are kw-only now - RedisCluster will no longer accept unknown arguments - RedisCluster will no longer accept url as an argument. Use RedisCluster.from_url - RedisCluster.require_full_coverage defaults to True - ClusterNode args except host, port, & server_type are kw-only now * async_cluster: remove kw-only requirement from client Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com> * More cherry picks to support latest module release * Fix KeyError in async cluster - initialize before execute multi key commands (#2439) * Fix KeyError in async cluster * link to issue * typo * fixing lint * fix json test Co-authored-by: Mehdi ABAAKOUK <sileht@sileht.net> Co-authored-by: Utkarsh Gupta <utkarshgupta137@gmail.com> Co-authored-by: Chayim I. Kirshen <c@kirshen.com>

* Add support for async graph * linters * fix docstring * Use retry mechanism in async version of Connection objects (#2271) * fix is_connected (#2278) * fix: workaround asyncio bug on connection reset by peer (#2259) Fixes #2237 * Fix crash: key expire while search (#2270) * fix expire while search * sleep * docs: Fix a few typos (#2274) * docs: Fix a few typos There are small typos in: - redis/cluster.py - redis/commands/core.py - redis/ocsp.py - tests/test_cluster.py Fixes: - Should read `validity` rather than `valididy`. - Should read `reinitialize` rather than `reinitilize`. - Should read `farthest` rather than `farest`. - Should read `commands` rather than `comamnds`. * Update core.py * async_cluster: fix concurrent pipeline (#2280) - each pipeline should create separate stacks for each node * Add support for TIMESERIES 1.8 (#2296) * Add support for timeseries 1.8 * fix info * linters * linters * fix info test * type hints * linters * Remove verbose logging from `redis-py/redis/cluster.py` (#2238) * removed the logging module and its corresponding methods * updated CHANGES * except block for RedisClusterException and BusyLoadingError removed * removed unused import (redis.exceptions.BusyLoadingError) * empty commit to re-trigger Actions workflow * replaced BaseException with Exception * empty commit to re-trigger Actions workflow * empty commit to re-trigger Actions workflow * redundant logic removed * re-trigger pipeline * reverted changes * re-trigger pipeline * except logic changed * redis stream example (#2269) * redis stream example * redis stream example on docs/examples.rst Co-authored-by: pedro.frazao <perl.pf@netcf.org> * Fix: `start_id` type for `XAUTOCLAIM` (#2257) * Changed start_id type for xautoclaim * Added to changes Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com> * Doc add timeseries example (#2267) * DOC add timeseries example * DOC add timeseries examples * Apply suggestions * Fix typo Detention period => Retention period Co-authored-by: Gauthier Imbert <gauthier@PC17> * Fix warnings and resource usage problems in asyncio unittests (#2258) * Use pytest-asyncio in auto mode Remove overly genereric `pytestmark=pytest.mark.asyncio` causing lots of warning noise * Use "Factories as Fixtures" test pattern for the `create_redis` fixture this fixture is now async, avoiding teardown problems with missing event loops. * Fix sporadic error on fast event loops, such as `--uvloop` * Close connection, even if "username" was in kwargs This fixes a resource usage warning in the async unittests. * Do async cleanup of acl passwords via a fixture * Remove unused import, fix whitespace * Fix test with missing "await" * Close pubsub objects after use in unittest Use a simple fixture where possible, otherwise manually call pubsub.close() * re-introduce `pytestmark=pytest.mark.asyncio` for python 3.6 * Use context manager to clean up connections in connection pool for unit tests * Provide asynccontextmanager for python 3.6 * make `test_late_subscribe()` more robuste * Catch a couple of additional leaked resources * Graph - add counters for removed labels and properties (#2292) * grpah - add counters for removed labels and properties * added mock graph result set statistics * docstrings for graph result set statistics * format * isort * moved docstrings into functions * cleaning up the readme and moving docs into readthedocs (#2291) * cleaning up the readme and moving docs into readthedocs * examples at the end as per pr comments * async_cluster: fix max_connections/ssl & improve args (#2217) * async_cluster: fix max_connections/ssl & improve args - set proper connection_class if ssl = True - pass max_connections/connection_class to ClusterNode - recreate startup_nodes to properly initialize - pass parser_class to Connection instead of changing it in on_connect - only pass redis_connect_func if read_from_replicas = True - add connection_error_retry_attempts parameter - skip is_connected check in acquire_connection as it is already checked in send_packed_command BREAKING: - RedisCluster args except host & port are kw-only now - RedisCluster will no longer accept unknown arguments - RedisCluster will no longer accept url as an argument. Use RedisCluster.from_url - RedisCluster.require_full_coverage defaults to True - ClusterNode args except host, port, & server_type are kw-only now * async_cluster: remove kw-only requirement from client Co-authored-by: dvora-h <67596500+dvora-h@users.noreply.github.com> * fix review comments * fix * fix review comments * fix review comments Co-authored-by: Chayim <chayim@users.noreply.github.com> Co-authored-by: szumka <106675199+szumka@users.noreply.github.com> Co-authored-by: Mehdi ABAAKOUK <sileht@sileht.net> Co-authored-by: Tim Gates <tim.gates@iress.com> Co-authored-by: Utkarsh Gupta <utkarshgupta137@gmail.com> Co-authored-by: Nial Daly <34862917+nialdaly@users.noreply.github.com> Co-authored-by: pedrofrazao <603718+pedrofrazao@users.noreply.github.com> Co-authored-by: pedro.frazao <perl.pf@netcf.org> Co-authored-by: Антон Безденежных <gamer392@yandex.ru> Co-authored-by: Iglesys <g.imbert34@gmail.com> Co-authored-by: Gauthier Imbert <gauthier@PC17> Co-authored-by: Kristján Valur Jónsson <sweskman@gmail.com> Co-authored-by: DvirDukhan <dvir@redis.com>

utkarshgupta137 force-pushed the async_cluster_args branch from e83f30e to 529252b Compare June 5, 2022 21:31

utkarshgupta137 force-pushed the async_cluster_args branch 2 times, most recently from 8e1ccf0 to 2174bce Compare June 5, 2022 21:40

utkarshgupta137 mentioned this pull request Jun 15, 2022

asyncio RedisCluster is not working for Amazon Elasticache #2233

Closed

utkarshgupta137 force-pushed the async_cluster_args branch 2 times, most recently from 0d8b0a7 to 5d63d75 Compare June 17, 2022 16:50

utkarshgupta137 force-pushed the async_cluster_args branch from 5d63d75 to 8f294ca Compare June 23, 2022 06:14

dvora-h reviewed Jun 23, 2022

View reviewed changes

utkarshgupta137 force-pushed the async_cluster_args branch from 8f294ca to b3d9cdd Compare July 24, 2022 13:02

async_cluster: remove kw-only requirement from client

8e066a2

dvora-h approved these changes Jul 24, 2022

View reviewed changes

utkarshgupta137 mentioned this pull request Jul 27, 2022

Support SSL in async Redis cluster client #2303

Closed

dvora-h added bug Bug breakingchange API or Breaking Change async labels Jul 27, 2022

Merge branch 'master' into async_cluster_args

79de404

dvora-h merged commit f665bd3 into redis:master Jul 27, 2022

karwinliu mentioned this pull request Jul 27, 2022

Wrong inheritance of the delete API with async Redis cluster pipeline #2305

Closed

utkarshgupta137 mentioned this pull request Nov 22, 2022

Version 4.3.5 #2469

Merged

6 tasks

utkarshgupta137 deleted the async_cluster_args branch December 1, 2022 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_cluster: fix max_connections/ssl & improve args #2217

async_cluster: fix max_connections/ssl & improve args #2217

utkarshgupta137 commented Jun 5, 2022

codecov-commenter commented Jun 5, 2022 •

edited

Loading

dvora-h Jun 23, 2022

utkarshgupta137 Jun 23, 2022

chayim Jun 23, 2022

utkarshgupta137 Jun 23, 2022

chayim Jun 26, 2022

utkarshgupta137 Jun 27, 2022

chayim Jul 24, 2022

utkarshgupta137 Jul 24, 2022

utkarshgupta137 commented Jul 6, 2022

utkarshgupta137 commented Jul 24, 2022

chayim commented Jul 24, 2022

utkarshgupta137 commented Jul 24, 2022

dvora-h commented Jul 24, 2022

utkarshgupta137 commented Sep 8, 2022

async_cluster: fix max_connections/ssl & improve args #2217

async_cluster: fix max_connections/ssl & improve args #2217

Conversation

utkarshgupta137 commented Jun 5, 2022

Pull Request check-list

Description of change

codecov-commenter commented Jun 5, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utkarshgupta137 commented Jul 6, 2022

utkarshgupta137 commented Jul 24, 2022

chayim commented Jul 24, 2022

utkarshgupta137 commented Jul 24, 2022

dvora-h commented Jul 24, 2022

utkarshgupta137 commented Sep 8, 2022

codecov-commenter commented Jun 5, 2022 •

edited

Loading