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

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
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.

None yet

3 participants