feat(cluster): #2448 add new DFLYMIGRATE ACK cmd #2582
Conversation
BorysTheDev
force-pushed
the
add_DFLYMIGRATE_ACK_cmd
branch
from
035362b
to
33f78da
Compare
|
|
||
| // TODO it's a bug need to rewrite with mutex or fix tl_cluster_config because it's not thread | ||
| // local | ||
| cluster_family_->cluster_config()->AddSlots(added_slots); |
There was a problem hiding this comment.
I think you should add the slots only if you receive OK on DFLYMIGRATE ACK
There was a problem hiding this comment.
Add slots is not a problem even if don't get OK, because it's an error processing situation. But if we add slots only after ok we can lose requests from the clients because the source node removes slots but the target node doesn't add them so nobody can process requests
| @@ -142,8 +142,7 @@ void ClusterSlotMigration::MainMigrationFb() { | |||
| << ec.message(); | |||
| } | |||
|
|
|||
| bool success = true; | |||
| if (success = IsFinalized(); success) { | |||
| if (IsFinalized()) { | |||
| state_ = MigrationState::C_FINISHED; | |||
There was a problem hiding this comment.
update state after you get ok?
There was a problem hiding this comment.
I think it's the correct place to set a new status, ACK cmd and OK doesn't affect slot migration process, and more like error processing
src/server/cluster/cluster_config.cc
Outdated
| @@ -298,6 +298,18 @@ shared_ptr<ClusterConfig> ClusterConfig::CreateFromConfig(string_view my_id, | |||
| return CreateFromConfig(my_id, config.value()); | |||
| } | |||
|
|
|||
| std::shared_ptr<ClusterConfig> ClusterConfig::CloneAndUpdate(const std::vector<SlotRange>& slots, | |||
There was a problem hiding this comment.
we need to update ClusterShards config_; here as well otherwise we will not reply with the relevant moved error
There was a problem hiding this comment.
yes I know, but if we use the new approach that we discuss we don;t need these changes
ACK cmd is the answer from a target node to a source node' FINALIZE cmd