-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Add consensus operation to restart shard transfer #3703
Conversation
// Abort and start transfer | ||
self.handle_transfer( | ||
collection_id.clone(), | ||
ShardTransferOperations::Abort { | ||
transfer: transfer.key(), | ||
reason: "restart transfer".into(), | ||
}, | ||
) | ||
.await?; | ||
self.handle_transfer(collection_id, ShardTransferOperations::Start(transfer)) | ||
.await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I just call abort and start right after each other, as part of a single operation. It keeps the implementation very simple.
Maybe there's more to it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks simple enough. LGTM.
let mut new_transfer = transfer.clone(); | ||
// Preserve sync flag from the old transfer | ||
new_transfer.sync = old_transfer.sync; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new stuff from my side, might require extra look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we do an OR, to sync if the old OR the new one has sync=true?
let mut new_transfer = transfer.clone(); | |
// Preserve sync flag from the old transfer | |
new_transfer.sync = old_transfer.sync; | |
// Preserve sync flag from the old transfer | |
let mut new_transfer = transfer.clone(); | |
new_transfer.sync |= old_transfer.sync; | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new_transfer.sync = old_transfer.sync || new_transfer.sync
. Don't use bitwise ops for logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've accidentally hit the green button.
I'm handling this here: #3728
* Add consensus operation to restart shard transfer with different config * Require shard transfer restart to have a changed configuration * implement api --------- Co-authored-by: generall <andrey@vasnetsov.com>
Tracked in: #3477
Add a consensus operation to restart a shard transfer with a different configuration.
This will make falling back to a different shard transfer method a whole lot nicer. We make sure to properly clean up and prepare for the different shard transfer method. Without it, we're manually managing and update shard replica set states to make transfers happen which is very fragile. Restarting this way also makes sure that the actual current transfer method and progress is properly reported.
The operation ensures that:
It is similar to calling abort/start right after each other, but in a single consensus operation.
If we'll merge this in 1.8, we'll be able to make use of it in Qdrant 1.9.
All Submissions:
dev
branch. Did you create your branch fromdev
?New Feature Submissions:
cargo +nightly fmt --all
command prior to submission?cargo clippy --all --all-features
command?Changes to Core Features: