Skip to content

Conversation

@sarthakaggarwal97
Copy link
Contributor

@sarthakaggarwal97 sarthakaggarwal97 commented Sep 12, 2025

Introduce a new config hash-seed which can be set only at startup and controls the hash seed for the server. This includes all hash tables. This change makes it so that both primaries and replicas will return the same results for SCAN/HSCAN/ZSCAN/SSCAN cursors. This is useful in order to make sure SCAN behaves correctly after a failover.

Resolves #4

@codecov
Copy link

codecov bot commented Sep 12, 2025

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.43%. Comparing base (a49d469) to head (f077e5e).
⚠️ Report is 2 commits behind head on unstable.

Files with missing lines Patch % Lines
src/config.c 50.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2608      +/-   ##
============================================
+ Coverage     72.39%   72.43%   +0.03%     
============================================
  Files           128      128              
  Lines         70245    70261      +16     
============================================
+ Hits          50853    50891      +38     
+ Misses        19392    19370      -22     
Files with missing lines Coverage Δ
src/server.c 88.44% <100.00%> (-0.02%) ⬇️
src/server.h 100.00% <ø> (ø)
src/util.c 66.53% <100.00%> (+0.36%) ⬆️
src/config.c 78.44% <50.00%> (-0.08%) ⬇️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sarthakaggarwal97 sarthakaggarwal97 marked this pull request as ready for review September 23, 2025 17:22
@sarthakaggarwal97 sarthakaggarwal97 marked this pull request as draft September 23, 2025 17:45
@sarthakaggarwal97 sarthakaggarwal97 force-pushed the fix-scan-consistency branch 3 times, most recently from b8ad08a to f0bccf7 Compare September 23, 2025 18:50
@sarthakaggarwal97 sarthakaggarwal97 marked this pull request as ready for review September 23, 2025 18:50
@sarthakaggarwal97 sarthakaggarwal97 force-pushed the fix-scan-consistency branch 3 times, most recently from 491c7bd to a0dcf78 Compare September 24, 2025 21:24
@sarthakaggarwal97 sarthakaggarwal97 marked this pull request as draft September 24, 2025 21:24
@sarthakaggarwal97 sarthakaggarwal97 force-pushed the fix-scan-consistency branch 4 times, most recently from a678a68 to bd920ed Compare September 24, 2025 23:29
@sarthakaggarwal97 sarthakaggarwal97 marked this pull request as ready for review September 25, 2025 00:57
@sarthakaggarwal97
Copy link
Contributor Author

@madolson I think it's ready for review 🙂

@sarthakaggarwal97 sarthakaggarwal97 force-pushed the fix-scan-consistency branch 2 times, most recently from 6251bbf to 158b61b Compare September 26, 2025 03:45
@sarthakaggarwal97
Copy link
Contributor Author

@madolson thanks for the review, I think I have addressed all the comments!

Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Looks closer, just some more minor comments.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Looks good in general, but I have a comment about the string-to-seed algorithm, which will becomes part of the interface, in the sense that if we change it in the future, it will break the scan cursor compatibility between nodes, so we may want think through this choice of algorithm.

Signed-off-by: Sarthak Aggarwal <[email protected]>
@sarthakaggarwal97 sarthakaggarwal97 force-pushed the fix-scan-consistency branch 2 times, most recently from c4a109e to 5f0aa05 Compare November 4, 2025 19:20
Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

LGTM!

sarthakaggarwal97 and others added 2 commits November 4, 2025 16:23
Co-authored-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
@madolson madolson added the release-notes This issue should get a line item in the release notes label Nov 5, 2025
@madolson madolson merged commit 32844b8 into valkey-io:unstable Nov 5, 2025
56 checks passed
@zuiderkwast
Copy link
Contributor

Should we mention this config on the documentation page for SCAN?

@sarthakaggarwal97
Copy link
Contributor Author

@zuiderkwast I also had this question. But when do we actually add the documentation, since the change is only in unstable and not released.

@zuiderkwast
Copy link
Contributor

But when do we actually add the documentation, since the change is only in unstable and not released.

We typically formulate things like this as "Starting with Valkey 9.1, it's possible to bla-bla-bla", because we want the docs to be useful regardless of which version a user is running, so we can add it any time once it's in unstable.

@sarthakaggarwal97
Copy link
Contributor Author

sure, makes sense. If people agree that we should add it, I will raise a PR asap.

@zuiderkwast zuiderkwast added the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Nov 5, 2025
@zuiderkwast
Copy link
Contributor

Yeah I think it'd be good to have. Maybe under this section? https://valkey.io/commands/scan/#scan-guarantees

We only need to add it for SCAN, since the other ones [HSZ]SCAN link to SCAN. Oh.. they don't, but they should(!). See https://valkey.io/commands/sscan/ . It was auto-linked in the redis times, but we should use an explicit link in the markdown.

eifrah-aws pushed a commit to eifrah-aws/valkey that referenced this pull request Nov 6, 2025
…y-io#2608)

Introduce a new config `hash-seed` which can be set only at startup and
controls the hash seed for the server. This includes all hash tables.
This change makes it so that both primaries and replicas will return the
same results for SCAN/HSCAN/ZSCAN/SSCAN cursors. This is useful in order
to make sure SCAN behaves correctly after a failover.

Resolves valkey-io#4

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
@zuiderkwast zuiderkwast removed the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Nov 6, 2025
zuiderkwast pushed a commit to valkey-io/valkey-doc that referenced this pull request Nov 6, 2025
zhijun42 pushed a commit to zhijun42/valkey that referenced this pull request Nov 15, 2025
…y-io#2608)

Introduce a new config `hash-seed` which can be set only at startup and
controls the hash seed for the server. This includes all hash tables.
This change makes it so that both primaries and replicas will return the
same results for SCAN/HSCAN/ZSCAN/SSCAN cursors. This is useful in order
to make sure SCAN behaves correctly after a failover.

Resolves valkey-io#4

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
zhijun42 pushed a commit to zhijun42/valkey that referenced this pull request Nov 15, 2025
…y-io#2608)

Introduce a new config `hash-seed` which can be set only at startup and
controls the hash seed for the server. This includes all hash tables.
This change makes it so that both primaries and replicas will return the
same results for SCAN/HSCAN/ZSCAN/SSCAN cursors. This is useful in order
to make sure SCAN behaves correctly after a failover.

Resolves valkey-io#4

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
zhijun42 pushed a commit to zhijun42/valkey that referenced this pull request Nov 28, 2025
…y-io#2608)

Introduce a new config `hash-seed` which can be set only at startup and
controls the hash seed for the server. This includes all hash tables.
This change makes it so that both primaries and replicas will return the
same results for SCAN/HSCAN/ZSCAN/SSCAN cursors. This is useful in order
to make sure SCAN behaves correctly after a failover.

Resolves valkey-io#4

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
zhijun42 pushed a commit to zhijun42/valkey that referenced this pull request Nov 28, 2025
…y-io#2608)

Introduce a new config `hash-seed` which can be set only at startup and
controls the hash seed for the server. This includes all hash tables.
This change makes it so that both primaries and replicas will return the
same results for SCAN/HSCAN/ZSCAN/SSCAN cursors. This is useful in order
to make sure SCAN behaves correctly after a failover.

Resolves valkey-io#4

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
zhijun42 pushed a commit to zhijun42/valkey that referenced this pull request Nov 28, 2025
…y-io#2608)

Introduce a new config `hash-seed` which can be set only at startup and
controls the hash seed for the server. This includes all hash tables.
This change makes it so that both primaries and replicas will return the
same results for SCAN/HSCAN/ZSCAN/SSCAN cursors. This is useful in order
to make sure SCAN behaves correctly after a failover.

Resolves valkey-io#4

---------

Signed-off-by: Sarthak Aggarwal <[email protected]>
Signed-off-by: Sarthak Aggarwal <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

major-decision-approved Major decision approved by TSC team release-notes This issue should get a line item in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

SCAN, SSCAN, HSCAN, and ZSCAN are not stable across failovers

5 participants