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

fix cluster extractFirstKey skip commandOptions() passed to args #2439

Merged
merged 5 commits into from
Apr 26, 2023

Conversation

carlhopf
Copy link
Contributor

Description

in cluster mode the node/slot is calculated based on extractFirstKey. skip commandOptions if passed as first argument

f.ex.

cluster.xRead(commandOptions({ isolated: true }), [{ key: 'foo', id: '0-0' }], { BLOCK: 1000 });

Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

@carlhopf carlhopf changed the title cluster extractFirstKey skip commandOptions() passed to args fix cluster extractFirstKey skip commandOptions() passed to args Mar 13, 2023
@leibale
Copy link
Collaborator

leibale commented Mar 14, 2023

@carlhopf thanks for contributing! Can you please add a test?
edit: on top of that, we're thinking of changing the way the command options are passed to a command to improve performance and readability. Since you are using this API, if you have some suggestions/ideas that will be very helpful :)
this is what we have in mind right now:

client.withOptions(...).ping();

@carlhopf
Copy link
Contributor Author

@leibale

added/updated the unit test: should assert cluster commands with commandOptions go to the correct hash slot without MOVED/ASK


the suggested chaining client.withOptions(...).ping(); looks like a great improvement to me 👍

@leibale
Copy link
Collaborator

leibale commented Apr 25, 2023

@carlhopf Sorry for the long delay... wanna review my changes? 🙏

@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (d65a641) 95.63% compared to head (db46727) 95.62%.

❗ Current head db46727 differs from pull request most recent head 17f59a1. Consider uploading reports for the commit 17f59a1 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2439      +/-   ##
==========================================
- Coverage   95.63%   95.62%   -0.01%     
==========================================
  Files         455      455              
  Lines        4579     4548      -31     
  Branches      528      520       -8     
==========================================
- Hits         4379     4349      -30     
+ Misses        129      128       -1     
  Partials       71       71              
Impacted Files Coverage Δ
packages/client/lib/client/pub-sub.ts 79.85% <ø> (ø)
packages/client/lib/commander.ts 95.45% <ø> (ø)
packages/client/lib/cluster/index.ts 81.90% <100.00%> (ø)

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@carlhopf
Copy link
Contributor Author

@leibale thanks for the review! good changes 👍🙏

@leibale leibale merged commit e1658ba into redis:master Apr 26, 2023
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.

3 participants