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

Node allow uncovered slots cscan #2815

Merged
merged 7 commits into from
Dec 22, 2024

Conversation

avifenesh
Copy link
Collaborator

@avifenesh avifenesh commented Dec 14, 2024

This PR is just node code, please check the last commit only.
For python and Rust please address in PR it rebased over.

Rebased over #2814
Adding uncovered slots cluster scan option to node

Issue link

This Pull Request is linked to issue #2689

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • [] CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Commits will be squashed upon merging.

@avifenesh avifenesh requested a review from a team as a code owner December 14, 2024 17:20
@avifenesh avifenesh self-assigned this Dec 14, 2024
@avifenesh avifenesh requested a review from jhpung December 14, 2024 17:20
@avifenesh avifenesh added node Node.js wrapper Feature Additional feature, big or small labels Dec 14, 2024
@avifenesh avifenesh added this to the 1.2.1 milestone Dec 14, 2024
@avifenesh avifenesh force-pushed the Node@allow-ucslots-Cscan branch 7 times, most recently from dfa092e to c7b68f0 Compare December 14, 2024 23:04
@ikolomi ikolomi removed this from the 1.2.1 milestone Dec 15, 2024
@avifenesh avifenesh force-pushed the Node@allow-ucslots-Cscan branch 2 times, most recently from 05aaab1 to 9c6b040 Compare December 15, 2024 10:46
Copy link
Collaborator

@jhpung jhpung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a changelog

node/src/Commands.ts Show resolved Hide resolved
node/tests/TestUtilities.ts Show resolved Hide resolved
node/tests/ScanTest.test.ts Show resolved Hide resolved
glide-core/redis-rs/redis/src/cluster_async/mod.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/commands/cluster_scan.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/commands/cluster_scan.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/commands/cluster_scan.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/commands/cluster_scan.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/commands/cluster_scan.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/commands/cluster_scan.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/commands/cluster_scan.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/commands/cluster_scan.rs Outdated Show resolved Hide resolved
@avifenesh avifenesh force-pushed the Node@allow-ucslots-Cscan branch 4 times, most recently from e0b2e32 to 2b772ab Compare December 22, 2024 11:30
Signed-off-by: avifenesh <[email protected]>
@avifenesh avifenesh force-pushed the Node@allow-ucslots-Cscan branch from 2b772ab to a844c88 Compare December 22, 2024 11:51
@avifenesh avifenesh force-pushed the Node@allow-ucslots-Cscan branch from 57c2c6c to 6518cf3 Compare December 22, 2024 12:02
@avifenesh avifenesh merged commit 210ba24 into valkey-io:main Dec 22, 2024
19 checks passed
yipin-chen pushed a commit that referenced this pull request Dec 24, 2024
* addresed comments

Signed-off-by: avifenesh <[email protected]>

* Go: add allow_non_covered_slots to ClusterScan and related commands

Signed-off-by: avifenesh <[email protected]>

* feat: Implement continuous slot scanning until next unscanned slot

Signed-off-by: jhpung <[email protected]>

* fix: improve slot scanning logic when address not found

Signed-off-by: jhpung <[email protected]>

* add allowNonCoveredSlots option to ScanOptions and update GlideClusterClient

Signed-off-by: avifenesh <[email protected]>

* test: add tests for GlideClusterClient scan with allowNonCoveredSlots option

Signed-off-by: avifenesh <[email protected]>

* refactor: enhance cluster readiness check and improve error handling in scan tests

Signed-off-by: avifenesh <[email protected]>

---------

Signed-off-by: avifenesh <[email protected]>
Signed-off-by: jhpung <[email protected]>
Co-authored-by: jhpung <[email protected]>
Signed-off-by: Yi-Pin Chen <[email protected]>
niharikabhavaraju pushed a commit to niharikabhavaraju/valkey-glide that referenced this pull request Dec 29, 2024
* addresed comments

Signed-off-by: avifenesh <[email protected]>

* Go: add allow_non_covered_slots to ClusterScan and related commands

Signed-off-by: avifenesh <[email protected]>

* feat: Implement continuous slot scanning until next unscanned slot

Signed-off-by: jhpung <[email protected]>

* fix: improve slot scanning logic when address not found

Signed-off-by: jhpung <[email protected]>

* add allowNonCoveredSlots option to ScanOptions and update GlideClusterClient

Signed-off-by: avifenesh <[email protected]>

* test: add tests for GlideClusterClient scan with allowNonCoveredSlots option

Signed-off-by: avifenesh <[email protected]>

* refactor: enhance cluster readiness check and improve error handling in scan tests

Signed-off-by: avifenesh <[email protected]>

---------

Signed-off-by: avifenesh <[email protected]>
Signed-off-by: jhpung <[email protected]>
Co-authored-by: jhpung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Additional feature, big or small node Node.js wrapper
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants