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.

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

Removes method from abort shard transfer operation #3803

Merged
merged 3 commits into from
Mar 26, 2024
Merged

Removes method from abort shard transfer operation #3803

merged 3 commits into from
Mar 26, 2024

Conversation

RPallas92
Copy link
Contributor

Tracked in: #3409

This PR adds a new AbortShardTransfer struct, to be used by the AbortTransfer operation.

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? - There is one, but the CI does not pass.

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?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@RPallas92 RPallas92 mentioned this pull request Mar 9, 2024
Closed
9 tasks
@RPallas92 RPallas92 marked this pull request as draft March 9, 2024 13:10
@RPallas92 RPallas92 marked this pull request as ready for review March 9, 2024 15:32
@RPallas92
Copy link
Contributor Author

@timvisee CI on my fork: https://github.com/RPallas92/qdrant/actions

@timvisee
Copy link
Member

Hi! Could you elaborate on the reasoning behind this?

Sadly we cannot really change existing (usage of) types because it breaks backwards compatibility.

@generall
Copy link
Member

@timvisee I am quite confident that changing those type is ok, as we are removing parameter, which under normal use should be always None.

@generall
Copy link
Member

It is, however, better to be merged closer to 1.9 release

@timvisee
Copy link
Member

@generall It was more about generated types in our gRPC clients, which will be broken now. But I'd be fine with including this in 1.9.

timvisee
timvisee approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@timvisee timvisee left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for picking this up!

Please see my review comment below.

lib/common/common/src/validation.rs Outdated Show resolved Hide resolved
@timvisee timvisee requested a review from generall March 11, 2024 15:23
@RPallas92
Copy link
Contributor Author

@timvisee feel free to merge, I don't have permissions

@timvisee
Copy link
Member

I'd like another explicit approval from @generall before merging this.

@timvisee
Copy link
Member

Merging now. Thank you @RPallas92!

@timvisee timvisee merged commit 265bfa2 into qdrant:dev Mar 26, 2024
17 checks passed
@RPallas92 RPallas92 deleted the dev branch March 26, 2024 14:34
timvisee pushed a commit that referenced this pull request Apr 22, 2024
@RPallas92 @timvisee
Removes method from abort shard transfer operation (#3803)
c35238f 
* Removes method from abort shard transfer operation

* Updates consensus test message assertion

* updates validation message
@timvisee
Copy link
Member

This has just been released as part of Qdrant 1.9. Thanks again!

@github-actions github-actions bot mentioned this pull request Jan 12, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timvisee timvisee timvisee approved these changes

@generall generall Awaiting requested review from generall

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

Successfully merging this pull request may close these issues.
3 participants
@RPallas92 @timvisee @generall