Fix hash ring initialization for custom sharding, prevent resharding panic#7517
Merged
Fix hash ring initialization for custom sharding, prevent resharding panic#7517
Conversation
Don't load the non key ring by default
When using auto sharding, require no shard key. When using custom sharding, require a shard key.
ffuugoo
approved these changes
Nov 11, 2025
Member
|
Do we have time for adding tests right now? |
Member
Author
Yes! |
Member
Author
|
@agourlay I've added some end to end tests to ensure this cannot happen again. Most critically it asserts that:
|
ffuugoo
reviewed
Nov 12, 2025
Co-authored-by: Roman Titov <ffuugoo@users.noreply.github.com>
agourlay
approved these changes
Nov 12, 2025
Member
agourlay
left a comment
There was a problem hiding this comment.
Thanks a lot for adding those great tests 👏
timvisee
added a commit
that referenced
this pull request
Nov 14, 2025
…panic (#7517) * Correctly rebuild hash rings, don't use default with custom sharding * Correctly initialize hash ring map with custom sharding Don't load the non key ring by default * Validate shard key for resharding more aggressively When using auto sharding, require no shard key. When using custom sharding, require a shard key. * Only pass sharding method into shard holder construction * Remove now obsolete debug assertion, we use same source config * Correctly reconstruct hash rings, also add shard IDs in auto mode * Add test, do not break consensus on invalid resharding down operation * Add tests for resharding down shard count precondition * Minor test tweaks * Improve debug assertion Co-authored-by: Roman Titov <ffuugoo@users.noreply.github.com> --------- Co-authored-by: Roman Titov <ffuugoo@users.noreply.github.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We currently don't initialize hash rings correctly when custom sharding is used.
Specifically, when custom sharding is used, we also initialize a hash ring for the
Nonekey. I'd argue this is not correct.This PR changes the shard holder loading behavior to not create the
Nonehash ring anymore. We now strictly define what kind of sharding is used, and derive the default rings from there.We had a production cluster panicking on resharding because no shard key was provided. Loading the hash rings as described in this PR would have prevented the panic with a graceful error. Applying this fix on a cluster that is already in a crash loop due to this problem will resolve the issue.
This PR adds some tests to ensure we check the shard count precondition before resharding. It also ensures we don't crash like we did before.
Tried with:
All Submissions:
devbranch. Did you create your branch fromdev?New Feature Submissions:
cargo +nightly fmt --allcommand prior to submission?cargo clippy --all --all-featurescommand?Changes to Core Features: