[5/5] Add Async MultiplexedClusterConnection #640

utkarshgupta137 · 2022-06-24T22:55:23Z

Fixes #421.

This is the final PR for adding async connection support for cluster.

TODO:

P.S. I've not added support for simple async connections, only multiplexed connections. I'm unaware of any benefits of the simple async connections.

utkarshgupta137 · 2022-07-03T06:13:56Z

@djc Can you do an initial review of these changes, after that I can proceed with adding tests/docs etc.

djc · 2022-07-04T09:41:56Z

Since this is pretty big and complex I'd like to see this split into smaller commits. Each commit should pass the tests and be formatted correctly). If you can describe it in a single bullet, it should be a single commit. Also please rebase on current main (and submit clippy fixes in a separate PR, please).

utkarshgupta137 · 2022-07-08T19:53:15Z

@djc @jaymell I've rebased this PR onto main. Now we've 3 commits, each of which I think should be separated into individual PRs, after some initial reviews here. I'm not sure how I can further break down the 3 commits & also get it passing tests at the same time.
I would recommend you hide whitespace changes & then review each commit. I'll try to see if I can sub-divide the commits this weekend, but I doubt it would make the review much more straightforward. I think it would be helpful if we could connect over discord or something so we can discuss changes in a rapid-fire manner.

djc · 2022-07-08T20:11:43Z

We have a Discord channel here: https://discord.gg/WHKcJK9AKP.

jaymell · 2022-07-23T05:32:28Z

I just want to second @djc's request that we significantly break these changes down into smaller commits. The work looks promising, but we need to be very careful about lumping so many unrelated changes together.

utkarshgupta137 · 2022-07-24T18:59:42Z

@jaymell @djc Raised a PR with the first commit broken up: #653. Once that is merged, I can start working on the next one.

utkarshgupta137 · 2022-07-28T01:41:53Z

@jaymell @djc Raised a PR with the 2nd commit broken up: #654. Now this PR only contains changes related to adding Async MultiplexedClusterConnection.

neogenie · 2022-08-21T15:42:01Z

Hi @djc @jaymell @utkarshgupta137

Any progress in this PR ? :)

djc · 2022-08-21T19:31:51Z

See #653.

- implement proper builder pattern by adding `new` & `builder` methods to ClusterClient and `new` & `build` methods to ClusterClientBuilder - deprecate & redirect `open` methods for ClusterClient & ClusterClientBuilder - move build method to ClusterClientBuilder

- this is to simplify passing multiple params to & inside ClusterConnection impl

- use plain host:port node naming, even for TLS as the tls mode/insecure flag will always be the same for all connections

- move the connect method inside impl - create new Connection directly instead of creating a Client first - convert to methods & use required params from self instead of passing - set connections within create_initial_connections & call refresh_slots

… cluster related params - remove unnecessary RefCell from params - this is to simplify passing multiple params to & inside the impl

- rename get_slots to parse_slots_response - move coverage checks & Slot creation logic into this fn - move CLUSTER SLOTS query logic out of this fn - this to easily reuse this function in the Async impl

- since we only operate in 100% slot coverage scenarios, there is no point in maintaining random connection logic, which is supposed to help us find missing nodes in the cluster topology - this also allows us to remove RefCell around connections & slots

- there is no point in using RefCell anymore

- this is to reuse the routing logic within ClusterConnection and also reuse it in async impl

…lots - set write/read timeouts just after creating connection - consolidate logic for create_initial_connections & refresh_slots connection creating/updating part into refresh_connections

- there is no point in having unnecessary indirection

- there is no point in having 2 different functions

…/supports_transactions

…pports_transactions

- remove ClusterPipeline & add re-exports for Pipeline/pipe - use `req_pipeline` instead of the packed version to avoid unpacking again for cluster - move cluster-specific pipeline checks into the new req_pipeline trait function

nihohit · 2024-02-01T11:53:11Z

@utkarshgupta137
Hi,
I see that you have multiple PRs for improving the cluster connection, that are based on old versions of the cluster. Are you interested in rebasing them, or can they be closed?

utkarshgupta137 · 2024-02-01T18:36:38Z

@utkarshgupta137 Hi, I see that you have multiple PRs for improving the cluster connection, that are based on old versions of the cluster. Are you interested in rebasing them, or can they be closed?

I've changed jobs & I don't use Redis anymore. I don't know the state of this crate at the moment & I'm not sure I've the bandwidth to make all the required changes. You're free to do with the PR as you like.

utkarshgupta137 force-pushed the cluster_async branch from c1ea5a2 to 899a863 Compare June 24, 2022 23:43

utkarshgupta137 changed the title ~~Cluster async~~ Add Async MultiplexedClusterConnection Jun 24, 2022

utkarshgupta137 mentioned this pull request Jul 4, 2022

Cluster: Create read_from_replicas option #635

Merged

utkarshgupta137 force-pushed the cluster_async branch from 899a863 to 02ccf07 Compare July 8, 2022 16:30

utkarshgupta137 force-pushed the cluster_async branch from 02ccf07 to 1af2a39 Compare July 28, 2022 01:34

utkarshgupta137 marked this pull request as ready for review July 28, 2022 01:36

utkarshgupta137 force-pushed the cluster_async branch from 1af2a39 to 967aa4c Compare July 28, 2022 01:39

utkarshgupta137 added 14 commits August 23, 2022 21:09

cluster_client: use ClusterParams struct to pass params

b5ec28e

- this is to simplify passing multiple params to & inside ClusterConnection impl

cluster: reorganise imports by module

79fe0ff

cluster: simplify TlsMode logic and add tls_insecure param

44a3de6

- use plain host:port node naming, even for TLS as the tls mode/insecure flag will always be the same for all connections

cluster: keep all params in ClusterParams struct & use it to pass all…

dee0504

… cluster related params - remove unnecessary RefCell from params - this is to simplify passing multiple params to & inside the impl

cluster: add read/write timeout params to builder

a12d245

cluster: add auto_reconnect param to builder

24544d4

cluster: add configurable connect timeout

34a8613

cluster: add configurable retries

cb94250

cluster: move slots response parsing logic to get_slots function

ad326cb

- rename get_slots to parse_slots_response - move coverage checks & Slot creation logic into this fn - move CLUSTER SLOTS query logic out of this fn - this to easily reuse this function in the Async impl

cluster: remove RefCell for connections/slot

5d14992

- there is no point in using RefCell anymore

cluster: consolidate routing logic into Routable trait

e943189

- this is to reuse the routing logic within ClusterConnection and also reuse it in async impl

utkarshgupta137 added 4 commits August 24, 2022 00:41

cluster: fix replica routing for execute_on_all_nodes

9d744d3

cluster: consolidate logic for create_initial_connections & refresh_s…

1088805

…lots - set write/read timeouts just after creating connection - consolidate logic for create_initial_connections & refresh_slots connection creating/updating part into refresh_connections

cluster: directly expose Slot fields instead of bare methods

0b9cb6b

- there is no point in having unnecessary indirection

cluster: merge send_all_commands & recv_all_commands

14f6d33

- there is no point in having 2 different functions

utkarshgupta137 changed the title ~~Add Async MultiplexedClusterConnection~~ [5/5] Add Async MultiplexedClusterConnection Aug 23, 2022

utkarshgupta137 added 5 commits August 24, 2022 00:58

aio: remove unnecessary futures dependency

5bfa0a6

aio: move connection creation logic from client to Runtime

de8a514

aio: fix ConnectionLike trait documentation & add supports_pipelining…

d4608c3

…/supports_transactions

sio: improve ConnectionLike trait documentation & add req_pipeline/su…

0884896

…pports_transactions

utkarshgupta137 force-pushed the cluster_async branch from 967aa4c to 24f58d7 Compare August 23, 2022 19:56

cluster: add Async MultiplexedClusterConnection

9968153

utkarshgupta137 force-pushed the cluster_async branch from 24f58d7 to 9968153 Compare August 23, 2022 20:13

nihohit mentioned this pull request Sep 9, 2023

[3/5] cluster: refactor/optimize for Async Cluster #653

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5/5] Add Async MultiplexedClusterConnection #640

[5/5] Add Async MultiplexedClusterConnection #640

utkarshgupta137 commented Jun 24, 2022 •

edited

utkarshgupta137 commented Jul 3, 2022

djc commented Jul 4, 2022

utkarshgupta137 commented Jul 8, 2022

djc commented Jul 8, 2022

jaymell commented Jul 23, 2022

utkarshgupta137 commented Jul 24, 2022

utkarshgupta137 commented Jul 28, 2022 •

edited

neogenie commented Aug 21, 2022 •

edited

djc commented Aug 21, 2022

nihohit commented Feb 1, 2024

utkarshgupta137 commented Feb 1, 2024

[5/5] Add Async MultiplexedClusterConnection #640

Are you sure you want to change the base?

[5/5] Add Async MultiplexedClusterConnection #640

Conversation

utkarshgupta137 commented Jun 24, 2022 • edited

utkarshgupta137 commented Jul 3, 2022

djc commented Jul 4, 2022

utkarshgupta137 commented Jul 8, 2022

djc commented Jul 8, 2022

jaymell commented Jul 23, 2022

utkarshgupta137 commented Jul 24, 2022

utkarshgupta137 commented Jul 28, 2022 • edited

neogenie commented Aug 21, 2022 • edited

djc commented Aug 21, 2022

nihohit commented Feb 1, 2024

utkarshgupta137 commented Feb 1, 2024

utkarshgupta137 commented Jun 24, 2022 •

edited

utkarshgupta137 commented Jul 28, 2022 •

edited

neogenie commented Aug 21, 2022 •

edited