Skip to content

commands/cluster: use pipeline to execute split commands#2230

Merged
dvora-h merged 1 commit into
redis:masterfrom
utkarshgupta137:cluster_non_atomic_pipeline
Jun 27, 2022
Merged

commands/cluster: use pipeline to execute split commands#2230
dvora-h merged 1 commit into
redis:masterfrom
utkarshgupta137:cluster_non_atomic_pipeline

Conversation

@utkarshgupta137

@utkarshgupta137 utkarshgupta137 commented Jun 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Since commands are split by slots, using a pipeline to send keys from the same node together provides a massive speedup, especially for the sync version.

Benchmarks using a batch of MGET 100 keys, MSET 100 keys, DEL 100 keys:

  • Sync (10 batches): 6.34s -> 0.26s
  • Async (100 batches): 9.34s -> 3.15s

Other changes:

  • allow passing target_nodes to pipeline commands
  • pass target_nodes for split commands
  • move READ_COMMANDS to commands/cluster to avoid import cycle
  • add types to list_or_args

@utkarshgupta137
commands/cluster: use pipeline to execute split commands
41016ff 
- allow passing target_nodes to pipeline commands

- move READ_COMMANDS to commands/cluster to avoid import cycle
- add types to list_or_args
@codecov-commenter

codecov-commenter commented Jun 14, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2230 (41016ff) into master (bedf3c8) will decrease coverage by 0.00%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #2230      +/-   ##
==========================================
- Coverage   91.83%   91.82%   -0.01%     
==========================================
  Files         108      108              
  Lines       27690    27685       -5     
==========================================
- Hits        25429    25423       -6     
- Misses       2261     2262       +1
Impacted Files Coverage Δ
redis/cluster.py 90.08% <83.33%> (+0.08%) ⬆️
redis/asyncio/cluster.py 89.96% <85.71%> (+0.05%) ⬆️
redis/commands/__init__.py 100.00% <100.00%> (ø)
redis/commands/cluster.py 93.64% <100.00%> (-0.57%) ⬇️
redis/commands/helpers.py 87.36% <100.00%> (+0.27%) ⬆️
tests/test_cluster.py 96.79% <0.00%> (-0.12%) ⬇️
redis/asyncio/connection.py 83.85% <0.00%> (+0.11%) ⬆️

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 bedf3c8...41016ff. Read the comment docs.

@dvora-h

dvora-h commented Jun 27, 2022

Copy link
Copy Markdown
Collaborator

@utkarshgupta137 Thanks! These things really improve the project.

@dvora-h dvora-h merged commit d7d4336 into redis:master Jun 27, 2022
@grippy grippy mentioned this pull request Aug 1, 2022
Merged
6 tasks
@utkarshgupta137 utkarshgupta137 deleted the cluster_non_atomic_pipeline branch December 1, 2022 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@dvora-h dvora-h dvora-h approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@utkarshgupta137 @codecov-commenter @dvora-h