Fix incorrect resharding transfer fraction for points to transfer #6259
Conversation
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
This comment was marked as resolved.
This comment was marked as resolved.
timvisee
commented
Mar 26, 2025
| // - shards: 3 -> 4 | ||
| // points: 33/33/33 -> 25/25/25/25 | ||
| // transfer fraction of each shard: 1/4/3 = 0.083 | ||
| Ordering::Less => (1.0 / to as f32) / (from as f32), |
There was a problem hiding this comment.
The final / from here was wrong. It adjusted the factor from shard level to collection level, which was not correct.
4f2e008 to
8fc4417
Compare
This was referenced Mar 26, 2025
Closed
KShivendu
approved these changes
Mar 26, 2025
Co-authored-by: Kumar Shivendu <kshivendu1@gmail.com>
5f56cc3 to
fe11d80
Compare
KShivendu
approved these changes
Mar 26, 2025
ffuugoo
approved these changes
Mar 26, 2025
Yes. With the previous (wrong 🤡) implementation it was a bit more complex, and the plan was to use it in two places. But now that it's simpler I'll move it directly into the call site instead. 👍 |
ffuugoo
reviewed
Mar 26, 2025
lib/collection/src/shards/transfer/resharding_stream_records.rs
Outdated
Show resolved
Hide resolved
timvisee
added a commit
that referenced
this pull request
Mar 31, 2025
) * Fix incorrect resharding transfer fraction for points to transfer * Explicitly handle case where from and to are the same * Update lib/collection/src/hash_ring.rs Co-authored-by: Kumar Shivendu <kshivendu1@gmail.com> * Move point fraction calculation right into the call site * Don't use intermediate f64 --------- Co-authored-by: Kumar Shivendu <kshivendu1@gmail.com>
Merged
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.
Fixes incorrect implementation in #6084.
The function to calculate the fraction of points transferred during a resharding transfer was incorrect. For resharding up it gave us the wrong result.
For resharding up, the returned fraction was for the total number of points in the collection, not a fraction for the number of points in the current shard like the function describes.
Long story short, this fixed the fraction calculation. The documentation with examples has been updated as well.
In practice this fixes our point count estimate in shard transfers. It's not critical, but helpful to do proper transfer estimations.
All Submissions:
devbranch. Did you create your branch fromdev?New Feature Submissions:
cargo +nightly fmt --allcommand prior to submission?cargo clippy --all --all-featurescommand?