Skip to content

Keep segment ID mapping when proxying all segments #7106

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 20, 2025
Merged

Keep segment ID mapping when proxying all segments #7106

merged 3 commits into from
Aug 20, 2025

Conversation

timvisee
Copy link
Member

@timvisee timvisee commented Aug 20, 2025 •
edited
Loading

The proxy_all_segments_and_apply/proxy_all_segments function did not preserve the existing segment ID mappings. It's important, because ongoing optimizations depend on this mapping to remain the same.

Fixes this debug assertion:

panicked at lib/collection/src/collection_manager/optimizers/segment_optimizer.rs:711:9: assertion `left == right` failed: swapped different number of proxies on unwrap, missing or incorrect segment IDs?
   left: 2
  right: 3

Contains some refactoring which makes the diff a bit harder to read. I've marked the critical part below.

All Submissions:

  • Contributions should target the dev branch. Did you create your branch from dev?
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
  3. Have you checked your code using cargo clippy --all --all-features command?

@timvisee timvisee requested a review from agourlay August 20, 2025 12:53
timvisee
timvisee commented Aug 20, 2025
View reviewed changes
lib/collection/src/collection_manager/holders/segment_holder.rs
Comment on lines +1142 to +1143
// We must keep existing segment IDs because ongoing optimizations might depend on the mapping being the same
write_segments.replace(segment_id, proxy)?;
Copy link
Member Author

@timvisee timvisee Aug 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Critical part.

This introduced new segment IDs before, now it preserves the existing mapping.

coderabbitai[bot]

This comment was marked as resolved.

Sign in to view

@qdrant qdrant deleted a comment from coderabbitai bot Aug 20, 2025
@agourlay agourlay merged commit 1d97e9a into dev Aug 20, 2025
16 checks passed
@agourlay agourlay deleted the segments-proxy-all-keep-mapping branch August 20, 2025 13:18
timvisee added a commit that referenced this pull request Aug 26, 2025
@timvisee
Keep segment ID mapping when proxying all segments (#7106)
c8d3958 
* When swapping existing segment, remove first to allow segment ID reuse

* Share original segment ID with proxies, keeps original mapping

* Add replace function, that directly replaces existing segment
@timvisee timvisee mentioned this pull request Aug 26, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@agourlay agourlay agourlay approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timvisee @agourlay