Skip to content

Commit

Permalink
fix: fix move error during migration finalization (#3253)
Browse files Browse the repository at this point in the history 
* fix: fix Move error during migration finalization
  • Loading branch information
@BorysTheDev
BorysTheDev authored Jul 2, 2024
1 parent 506ecbc commit 84814a7
Show file tree
Hide file tree
Showing 10 changed files with 185 additions and 111 deletions.
30 changes: 19 additions & 11 deletions src/server/cluster/cluster_config.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ shared_ptr<ClusterConfig> ClusterConfig::CreateFromConfig(string_view my_id,

shared_ptr<ClusterConfig> result(new ClusterConfig());

result->my_id_ = my_id;
result->config_ = config;

for (const auto& shard : result->config_) {
Expand All @@ -101,10 +102,10 @@ shared_ptr<ClusterConfig> ClusterConfig::CreateFromConfig(string_view my_id,
result->my_outgoing_migrations_ = shard.migrations;
} else {
for (const auto& m : shard.migrations) {
if (my_id == m.node_id) {
if (my_id == m.node_info.id) {
auto incoming_migration = m;
// for incoming migration we need the source node
incoming_migration.node_id = shard.master.id;
incoming_migration.node_info.id = shard.master.id;
result->my_incoming_migrations_.push_back(std::move(incoming_migration));
}
}
Expand Down Expand Up @@ -132,7 +133,7 @@ optional<SlotRanges> GetClusterSlotRanges(const JsonType& slots) {
return nullopt;
}

SlotRanges ranges;
std::vector<SlotRange> ranges;

for (const auto& range : slots.array_range()) {
if (!range.is_object()) {
Expand All @@ -149,7 +150,7 @@ optional<SlotRanges> GetClusterSlotRanges(const JsonType& slots) {
ranges.push_back({.start = start.value(), .end = end.value()});
}

return ranges;
return SlotRanges(ranges);
}

optional<ClusterNodeInfo> ParseClusterNode(const JsonType& json) {
Expand Down Expand Up @@ -211,10 +212,10 @@ optional<std::vector<MigrationInfo>> ParseMigrations(const JsonType& json) {
return nullopt;
}

res.emplace_back(MigrationInfo{.slot_ranges = std::move(*slots),
.node_id = node_id.as_string(),
.ip = ip.as_string(),
.port = *port});
res.emplace_back(MigrationInfo{
.slot_ranges = std::move(*slots),
.node_info =
ClusterNodeInfo{.id = node_id.as_string(), .ip = ip.as_string(), .port = *port}});
}
return res;
}
Expand Down Expand Up @@ -316,10 +317,17 @@ ClusterNodeInfo ClusterConfig::GetMasterNodeForSlot(SlotId id) const {
CHECK_LE(id, cluster::kMaxSlotNum) << "Requesting a non-existing slot id " << id;

for (const auto& shard : config_) {
for (const auto& range : shard.slot_ranges) {
if (id >= range.start && id <= range.end) {
return shard.master;
if (shard.slot_ranges.Contains(id)) {
if (shard.master.id == my_id_) {
// The only reason why this function call and shard.master == my_id_ is the slot was
// migrated
for (const auto& m : shard.migrations) {
if (m.slot_ranges.Contains(id)) {
return m.node_info;
}
}
}
return shard.master;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/server/cluster/cluster_config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include <vector>

#include "src/server/cluster/slot_set.h"
#include "src/server/common.h"

namespace dfly::cluster {

Expand Down Expand Up @@ -59,6 +58,7 @@ class ClusterConfig {

ClusterConfig() = default;

std::string my_id_;
ClusterShardInfos config_;

SlotSet my_slots_;
Expand Down
40 changes: 20 additions & 20 deletions src/server/cluster/cluster_config_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ TEST_F(ClusterConfigTest, ConfigSetInvalidEmpty) {

TEST_F(ClusterConfigTest, ConfigSetInvalidMissingSlots) {
EXPECT_EQ(ClusterConfig::CreateFromConfig(
kMyId, {{.slot_ranges = {{.start = 0, .end = 16000}},
kMyId, {{.slot_ranges = SlotRanges({{.start = 0, .end = 16000}}),
.master = {.id = "other", .ip = "192.168.0.100", .port = 7000},
.replicas = {},
.migrations = {}}}),
Expand All @@ -105,11 +105,11 @@ TEST_F(ClusterConfigTest, ConfigSetInvalidMissingSlots) {

TEST_F(ClusterConfigTest, ConfigSetInvalidDoubleBookedSlot) {
EXPECT_EQ(ClusterConfig::CreateFromConfig(
kMyId, {{.slot_ranges = {{.start = 0, .end = 0x3FFF}},
kMyId, {{.slot_ranges = SlotRanges({{.start = 0, .end = 0x3FFF}}),
.master = {.id = "other", .ip = "192.168.0.100", .port = 7000},
.replicas = {},
.migrations = {}},
{.slot_ranges = {{.start = 0, .end = 0}},
{.slot_ranges = SlotRanges({{.start = 0, .end = 0}}),
.master = {.id = "other2", .ip = "192.168.0.101", .port = 7001},
.replicas = {},
.migrations = {}}}),
Expand All @@ -118,7 +118,7 @@ TEST_F(ClusterConfigTest, ConfigSetInvalidDoubleBookedSlot) {

TEST_F(ClusterConfigTest, ConfigSetInvalidSlotId) {
EXPECT_EQ(ClusterConfig::CreateFromConfig(
kMyId, {{.slot_ranges = {{.start = 0, .end = 0x3FFF + 1}},
kMyId, {{.slot_ranges = SlotRanges({{.start = 0, .end = 0x3FFF + 1}}),
.master = {.id = "other", .ip = "192.168.0.100", .port = 7000},
.replicas = {},
.migrations = {}}}),
Expand All @@ -127,7 +127,7 @@ TEST_F(ClusterConfigTest, ConfigSetInvalidSlotId) {

TEST_F(ClusterConfigTest, ConfigSetOk) {
auto config = ClusterConfig::CreateFromConfig(
kMyId, {{.slot_ranges = {{.start = 0, .end = 0x3FFF}},
kMyId, {{.slot_ranges = SlotRanges({{.start = 0, .end = 0x3FFF}}),
.master = {.id = "other", .ip = "192.168.0.100", .port = 7000},
.replicas = {},
.migrations = {}}});
Expand All @@ -139,7 +139,7 @@ TEST_F(ClusterConfigTest, ConfigSetOk) {

TEST_F(ClusterConfigTest, ConfigSetOkWithReplica) {
auto config = ClusterConfig::CreateFromConfig(
kMyId, {{.slot_ranges = {{.start = 0, .end = 0x3FFF}},
kMyId, {{.slot_ranges = SlotRanges({{.start = 0, .end = 0x3FFF}}),
.master = {.id = "other-master", .ip = "192.168.0.100", .port = 7000},
.replicas = {{.id = "other-replica", .ip = "192.168.0.101", .port = 7001}},
.migrations = {}}});
Expand All @@ -150,21 +150,21 @@ TEST_F(ClusterConfigTest, ConfigSetOkWithReplica) {

TEST_F(ClusterConfigTest, ConfigSetMultipleInstances) {
auto config = ClusterConfig::CreateFromConfig(
kMyId, {{.slot_ranges = {{.start = 0, .end = 5'000}},
kMyId, {{.slot_ranges = SlotRanges({{.start = 0, .end = 5'000}}),
.master = {.id = "other-master", .ip = "192.168.0.100", .port = 7000},
.replicas = {{.id = "other-replica", .ip = "192.168.0.101", .port = 7001}},
.migrations = {}},
{.slot_ranges = {{.start = 5'001, .end = 10'000}},
{.slot_ranges = SlotRanges({{.start = 5'001, .end = 10'000}}),
.master = {.id = kMyId, .ip = "192.168.0.102", .port = 7002},
.replicas = {{.id = "other-replica2", .ip = "192.168.0.103", .port = 7003}},
.migrations = {}},
{.slot_ranges = {{.start = 10'001, .end = 0x3FFF}},
{.slot_ranges = SlotRanges({{.start = 10'001, .end = 0x3FFF}}),
.master = {.id = "other-master3", .ip = "192.168.0.104", .port = 7004},
.replicas = {{.id = "other-replica3", .ip = "192.168.0.105", .port = 7005}},
.migrations = {}}});
EXPECT_NE(config, nullptr);
SlotSet owned_slots = config->GetOwnedSlots();
EXPECT_EQ(owned_slots.ToSlotRanges().size(), 1);
EXPECT_EQ(owned_slots.ToSlotRanges().Size(), 1);
EXPECT_EQ(owned_slots.Count(), 5'000);

{
Expand Down Expand Up @@ -481,8 +481,8 @@ TEST_F(ClusterConfigTest, ConfigSetMigrations) {
auto config1 = ClusterConfig::CreateFromConfig("id0", config_str);
EXPECT_EQ(
config1->GetNewOutgoingMigrations(nullptr),
(std::vector<MigrationInfo>{
{.slot_ranges = {{7000, 8000}}, .node_id = "id1", .ip = "127.0.0.1", .port = 9001}}));
(std::vector<MigrationInfo>{{.slot_ranges = SlotRanges({{7000, 8000}}),
.node_info = {.id = "id1", .ip = "127.0.0.1", .port = 9001}}}));

EXPECT_TRUE(config1->GetFinishedOutgoingMigrations(nullptr).empty());
EXPECT_TRUE(config1->GetNewIncomingMigrations(nullptr).empty());
Expand All @@ -491,8 +491,8 @@ TEST_F(ClusterConfigTest, ConfigSetMigrations) {
auto config2 = ClusterConfig::CreateFromConfig("id1", config_str);
EXPECT_EQ(
config2->GetNewIncomingMigrations(nullptr),
(std::vector<MigrationInfo>{
{.slot_ranges = {{7000, 8000}}, .node_id = "id0", .ip = "127.0.0.1", .port = 9001}}));
(std::vector<MigrationInfo>{{.slot_ranges = SlotRanges({{7000, 8000}}),
.node_info = {.id = "id0", .ip = "127.0.0.1", .port = 9001}}}));

EXPECT_TRUE(config2->GetFinishedOutgoingMigrations(nullptr).empty());
EXPECT_TRUE(config2->GetNewOutgoingMigrations(nullptr).empty());
Expand Down Expand Up @@ -523,16 +523,16 @@ TEST_F(ClusterConfigTest, ConfigSetMigrations) {

EXPECT_EQ(
config4->GetFinishedOutgoingMigrations(config1),
(std::vector<MigrationInfo>{
{.slot_ranges = {{7000, 8000}}, .node_id = "id1", .ip = "127.0.0.1", .port = 9001}}));
(std::vector<MigrationInfo>{{.slot_ranges = SlotRanges({{7000, 8000}}),
.node_info = {.id = "id1", .ip = "127.0.0.1", .port = 9001}}}));
EXPECT_TRUE(config4->GetNewIncomingMigrations(config1).empty());
EXPECT_TRUE(config4->GetFinishedIncomingMigrations(config1).empty());
EXPECT_TRUE(config4->GetNewOutgoingMigrations(config1).empty());

EXPECT_EQ(
config5->GetFinishedIncomingMigrations(config2),
(std::vector<MigrationInfo>{
{.slot_ranges = {{7000, 8000}}, .node_id = "id0", .ip = "127.0.0.1", .port = 9001}}));
(std::vector<MigrationInfo>{{.slot_ranges = SlotRanges({{7000, 8000}}),
.node_info = {.id = "id0", .ip = "127.0.0.1", .port = 9001}}}));
EXPECT_TRUE(config5->GetNewIncomingMigrations(config2).empty());
EXPECT_TRUE(config5->GetFinishedOutgoingMigrations(config2).empty());
EXPECT_TRUE(config5->GetNewOutgoingMigrations(config2).empty());
Expand Down Expand Up @@ -589,7 +589,7 @@ TEST_F(ClusterConfigTest, SlotSetAPI) {
ss.Set(5010, true);
EXPECT_EQ(ss.ToSlotRanges(), SlotRanges({{0, 2000}, {5010, 5010}}));

ss.Set({SlotRange{5000, 5100}}, true);
ss.Set(SlotRanges({{5000, 5100}}), true);
EXPECT_EQ(ss.ToSlotRanges(), SlotRanges({{0, 2000}, {5000, 5100}}));

ss.Set(5050, false);
Expand All @@ -598,7 +598,7 @@ TEST_F(ClusterConfigTest, SlotSetAPI) {
ss.Set(5500, false);
EXPECT_EQ(ss.ToSlotRanges(), SlotRanges({{0, 2000}, {5000, 5049}, {5051, 5100}}));

ss.Set({SlotRange{5090, 5100}}, false);
ss.Set(SlotRanges({{5090, 5100}}), false);
EXPECT_EQ(ss.ToSlotRanges(), SlotRanges({{0, 2000}, {5000, 5049}, {5051, 5089}}));

SlotSet ss1(SlotRanges({{1001, 2000}}));
Expand Down
30 changes: 30 additions & 0 deletions src/server/cluster/cluster_defs.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@ extern "C" {
#include "redis/crc16.h"
}

#include <absl/strings/str_cat.h>
#include <absl/strings/str_join.h>

#include "base/flags.h"
#include "base/logging.h"
#include "cluster_defs.h"
#include "slot_set.h"
#include "src/server/common.h"

using namespace std;
Expand All @@ -15,6 +19,32 @@ ABSL_FLAG(string, cluster_mode, "",
"'emulated', 'yes' or ''");

namespace dfly::cluster {
std::string SlotRange::ToString() const {
return absl::StrCat("[", start, ", ", end, "]");
}

SlotRanges::SlotRanges(std::vector<SlotRange> ranges) : ranges_(std::move(ranges)) {
std::sort(ranges_.begin(), ranges_.end());
}

void SlotRanges::Merge(const SlotRanges& sr) {
// TODO rewrite it
SlotSet slots(*this);
slots.Set(sr, true);
ranges_ = std::move(slots.ToSlotRanges().ranges_);
}

std::string SlotRanges::ToString() const {
return absl::StrJoin(ranges_, ", ", [](std::string* out, SlotRange range) {
absl::StrAppend(out, range.ToString());
});
}

std::string MigrationInfo::ToString() const {
return absl::StrCat(node_info.id, ",", node_info.ip, ":", node_info.port, " (",
slot_ranges.ToString(), ")");
}

namespace {
enum class ClusterMode {
kUninitialized,
Expand Down
83 changes: 60 additions & 23 deletions src/server/cluster/cluster_defs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@

#pragma once

#include <absl/strings/str_cat.h>
#include <absl/strings/str_join.h>

#include <memory>
#include <cstdint>
#include <string>
#include <string_view>
#include <vector>
Expand All @@ -24,45 +21,85 @@ struct SlotRange {
SlotId start = 0;
SlotId end = 0;

bool operator==(const SlotRange& r) const {
bool operator==(const SlotRange& r) const noexcept {
return start == r.start && end == r.end;
}
bool IsValid() {
return start <= end && start <= kMaxSlotId && end <= kMaxSlotId;

bool operator<(const SlotRange& r) const noexcept {
return start < r.start || (start == r.start && end < r.end);
}

std::string ToString() const {
return absl::StrCat("[", start, ", ", end, "]");
bool IsValid() const noexcept {
return start <= end && start <= kMaxSlotId && end <= kMaxSlotId;
}

static std::string ToString(const std::vector<SlotRange>& ranges) {
return absl::StrJoin(ranges, ", ", [](std::string* out, SlotRange range) {
absl::StrAppend(out, range.ToString());
});
bool Contains(SlotId id) const noexcept {
return id >= start && id <= end;
}

std::string ToString() const;
};

using SlotRanges = std::vector<SlotRange>;
class SlotRanges {
public:
SlotRanges() = default;
explicit SlotRanges(std::vector<SlotRange> ranges);

bool Contains(SlotId id) const noexcept {
for (const auto& sr : ranges_) {
if (sr.Contains(id))
return true;
}
return false;
}

size_t Size() const noexcept {
return ranges_.size();
}

bool Empty() const noexcept {
return ranges_.empty();
}

void Merge(const SlotRanges& sr);

bool operator==(const SlotRanges& r) const noexcept {
return ranges_ == r.ranges_;
}

std::string ToString() const;

auto begin() const noexcept {
return ranges_.cbegin();
}

auto end() const noexcept {
return ranges_.cend();
}

private:
std::vector<SlotRange> ranges_;
};

struct ClusterNodeInfo {
std::string id;
std::string ip;
uint16_t port = 0;

bool operator==(const ClusterNodeInfo& r) const noexcept {
return port == r.port && ip == r.ip && id == r.id;
}
};

struct MigrationInfo {
std::vector<SlotRange> slot_ranges;
std::string node_id;
std::string ip;
uint16_t port = 0;
SlotRanges slot_ranges;
ClusterNodeInfo node_info;

bool operator==(const MigrationInfo& r) const {
return ip == r.ip && port == r.port && slot_ranges == r.slot_ranges && node_id == r.node_id;
bool operator==(const MigrationInfo& r) const noexcept {
return node_info == r.node_info && slot_ranges == r.slot_ranges;
}

std::string ToString() const {
return absl::StrCat(node_id, ",", ip, ":", port, " (", SlotRange::ToString(slot_ranges), ")");
}
std::string ToString() const;
};

struct ClusterShardInfo {
Expand Down
Loading

0 comments on commit 84814a7

Please sign in to comment.