-
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
Send updates to the new shard during resharding #4256
Conversation
e39346c
to
5a5ad7b
Compare
0ecbf7a
to
83c310a
Compare
This still feels a bit "hacky", IMO, but I don't want to spend more time on this RN. I propose we merge it, and then improve on it once we started working on the resharding driver. |
lib/collection/src/collection/mod.rs
Outdated
// TODO(resharding): 🤔 | ||
if current_state == Some(ReplicaState::Resharding) && state == ReplicaState::Dead { | ||
let shard_key = shard_holder | ||
.get_shard_id_to_key_mapping() | ||
.get(&shard_id) | ||
.cloned(); | ||
|
||
drop(shard_holder); | ||
|
||
self.abort_resharding(peer_id, shard_id, shard_key).await?; | ||
|
||
// TODO(resharding): Abort all resharding transfers!? | ||
|
||
return Ok(()); | ||
} |
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.
How about doing this in sync_local_state
, like we do with shard transfers?
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.
Can you elaborate? abort_resharding
does not send any consensus messages. It actually aborts resharding locally. We also abort shard transfers on Dead
state in the same way (just a few lines below selected snippet).
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 missed that bit.
Where should we produce the abort resharding concencus call though? Should the driver itself be responsible for this?
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.
Either user aborts explicitly, or the driver (e.g., if one of the transfers failed). When update fails we just propose peer Dead
and abort when applying this state change.
3f7a5e1
to
11c43b2
Compare
83c310a
to
fd13bb0
Compare
fd13bb0
to
81d11fe
Compare
...and tweak `ShardReplicaSet::update_impl` to handle single replica in `Resharding` state without an error
7ed00ed
to
b23126f
Compare
Tracked in #4213.
This PR enables sending update to the created shard and adds basic error handling for failed updates.
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: