Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

utkarshgupta137
Copy link
Contributor

@utkarshgupta137 utkarshgupta137 commented Jun 14, 2022

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

- 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
Copy link

codecov-commenter commented Jun 14, 2022

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
Copy link
Collaborator

dvora-h commented Jun 27, 2022

@utkarshgupta137 Thanks! These things really improve the project.

@dvora-h dvora-h merged commit d7d4336 into redis:master Jun 27, 2022
@utkarshgupta137 utkarshgupta137 deleted the cluster_non_atomic_pipeline branch December 1, 2022 08:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants