You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "acelyc111 (via GitHub)" <gi...@apache.org> on 2023/05/17 10:55:51 UTC
[GitHub] [incubator-pegasus] acelyc111 opened a new pull request, #1477: refactor: improve the single-responsibility of class fs_manager
acelyc111 opened a new pull request, #1477:
URL: https://github.com/apache/incubator-pegasus/pull/1477
https://github.com/apache/incubator-pegasus/issues/1383
- move some functions from replica_stub to fs_manager
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1477: refactor: improve the single-responsibility of class fs_manager
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1477:
URL: https://github.com/apache/incubator-pegasus/pull/1477#discussion_r1203667395
##########
src/replica/test/mock_utils.h:
##########
@@ -231,23 +231,27 @@ class mock_replica : public replica
};
typedef dsn::ref_ptr<mock_replica> mock_replica_ptr;
-inline std::unique_ptr<mock_replica> create_mock_replica(replica_stub *stub,
- int appid = 1,
- int partition_index = 1,
- const char *dir = "./")
+inline std::unique_ptr<mock_replica>
+create_mock_replica(replica_stub *stub, int appid = 1, int partition_index = 1)
{
gpid gpid(appid, partition_index);
app_info app_info;
app_info.app_type = "replica";
app_info.app_name = "temp";
- return std::make_unique<mock_replica>(stub, gpid, app_info, dir);
+ dir_node *dn =
Review Comment:
```suggestion
const auto * const dn =
```
##########
src/replica/test/replica_disk_migrate_test.cpp:
##########
@@ -362,31 +362,44 @@ TEST_F(replica_disk_migrate_test, disk_migrate_replica_update)
}
}
+// Test load from new replica dir failed, then fall back to load from origin dir succeed,
+// and then mark the "new" replica dir as ".gar".
TEST_F(replica_disk_migrate_test, disk_migrate_replica_open)
{
+ gpid test_pid(app_info_1.app_id, 4);
+
+ // Suppose gpid 1.4 is migrated from tag_2 to tag_empty_1.
auto &request = *fake_migrate_rpc.mutable_request();
- request.pid = dsn::gpid(app_info_1.app_id, 4);
+ request.pid = test_pid;
request.origin_disk = "tag_2";
request.target_disk = "tag_empty_1";
- remove_mock_dir_node(request.origin_disk);
+ // Remove the gpid 1.4 dir which is created in constructor.
+ const std::string kReplicaOriginDir =
+ fmt::format("./{}/{}.replica", request.origin_disk, request.pid.to_string());
+ utils::filesystem::remove_path(kReplicaOriginDir);
+ stub->get_fs_manager()->remove_replica(test_pid);
+
+ // Create the related dirs.
const std::string kReplicaOriginSuffixDir = fmt::format(
"./{}/{}.replica.disk.migrate.ori/", request.origin_disk, request.pid.to_string());
const std::string kReplicaNewDir =
fmt::format("./{}/{}.replica/", request.target_disk, request.pid.to_string());
utils::filesystem::create_directory(kReplicaOriginSuffixDir);
utils::filesystem::create_directory(kReplicaNewDir);
+ // The replica can be opened nomally. In fact, the original dir is opened, and the new dir will
+ // be garbage.
fail::cfg("mock_replica_load", "return()");
- const std::string kReplicaOriginDir =
- fmt::format("./{}/{}.replica", request.origin_disk, request.pid.to_string());
- const std::string kReplicaGarDir =
- fmt::format("./{}/{}.replica.gar", request.target_disk, request.pid.to_string());
open_replica(app_info_1, request.pid);
+ // Check it works as expected.
+ const std::string kReplicaGarDir =
Review Comment:
```suggestion
const auto kReplicaGarDir =
```
##########
src/replica/test/mock_utils.h:
##########
@@ -231,23 +231,27 @@ class mock_replica : public replica
};
typedef dsn::ref_ptr<mock_replica> mock_replica_ptr;
-inline std::unique_ptr<mock_replica> create_mock_replica(replica_stub *stub,
- int appid = 1,
- int partition_index = 1,
- const char *dir = "./")
+inline std::unique_ptr<mock_replica>
+create_mock_replica(replica_stub *stub, int appid = 1, int partition_index = 1)
{
gpid gpid(appid, partition_index);
app_info app_info;
app_info.app_type = "replica";
app_info.app_name = "temp";
- return std::make_unique<mock_replica>(stub, gpid, app_info, dir);
+ dir_node *dn =
+ stub->get_fs_manager()->create_replica_dir_if_necessary(app_info.app_type.c_str(), gpid);
+ CHECK_NOTNULL(dn, "");
+ const auto replica_path = dn->replica_dir(app_info.app_type.c_str(), gpid);
Review Comment:
```suggestion
const auto replica_path = dn->replica_dir(app_info.app_type, gpid);
```
##########
src/replica/test/replica_disk_migrate_test.cpp:
##########
@@ -362,31 +362,44 @@ TEST_F(replica_disk_migrate_test, disk_migrate_replica_update)
}
}
+// Test load from new replica dir failed, then fall back to load from origin dir succeed,
+// and then mark the "new" replica dir as ".gar".
TEST_F(replica_disk_migrate_test, disk_migrate_replica_open)
{
+ gpid test_pid(app_info_1.app_id, 4);
+
+ // Suppose gpid 1.4 is migrated from tag_2 to tag_empty_1.
auto &request = *fake_migrate_rpc.mutable_request();
- request.pid = dsn::gpid(app_info_1.app_id, 4);
+ request.pid = test_pid;
request.origin_disk = "tag_2";
request.target_disk = "tag_empty_1";
- remove_mock_dir_node(request.origin_disk);
+ // Remove the gpid 1.4 dir which is created in constructor.
+ const std::string kReplicaOriginDir =
+ fmt::format("./{}/{}.replica", request.origin_disk, request.pid.to_string());
Review Comment:
```suggestion
fmt::format("./{}/{}.replica", request.origin_disk, request.pid);
```
##########
src/common/fs_manager.cpp:
##########
@@ -236,44 +277,40 @@ void fs_manager::add_replica(const gpid &pid, const std::string &pid_dir)
LOG_INFO("{}: add gpid({}) to dir_node({})", dsn_primary_address(), pid, dn->tag);
}
-void fs_manager::allocate_dir(const gpid &pid, const std::string &type, /*out*/ std::string &dir)
+dir_node *fs_manager::find_best_dir_for_new_replica(const gpid &pid) const
{
- char buffer[256];
- sprintf(buffer, "%d.%d.%s", pid.get_app_id(), pid.get_partition_index(), type.c_str());
-
- zauto_write_lock l(_lock);
-
dir_node *selected = nullptr;
-
- unsigned least_app_replicas_count = 0;
- unsigned least_total_replicas_count = 0;
-
- for (auto &n : _dir_nodes) {
- CHECK(!n->has(pid), "gpid({}) already in dir_node({})", pid, n->tag);
- unsigned app_replicas = n->replicas_count(pid.get_app_id());
- unsigned total_replicas = n->replicas_count();
-
- if (selected == nullptr || least_app_replicas_count > app_replicas) {
- least_app_replicas_count = app_replicas;
- least_total_replicas_count = total_replicas;
- selected = n.get();
- } else if (least_app_replicas_count == app_replicas &&
- least_total_replicas_count > total_replicas) {
- least_total_replicas_count = total_replicas;
- selected = n.get();
+ uint64_t least_app_replicas_count = 0;
+ uint64_t least_total_replicas_count = 0;
+ {
+ zauto_write_lock l(_lock);
+ // Try to find the dir_node with the least replica count.
+ for (const auto &dn : _dir_nodes) {
+ CHECK(!dn->has(pid), "gpid({}) already exists in dir_node({})", pid, dn->tag);
+ uint64_t app_replicas_count = dn->replicas_count(pid.get_app_id());
+ uint64_t total_replicas_count = dn->replicas_count();
+
+ if (selected == nullptr || least_app_replicas_count > app_replicas_count) {
+ least_app_replicas_count = app_replicas_count;
+ least_total_replicas_count = total_replicas_count;
+ selected = dn.get();
+ } else if (least_app_replicas_count == app_replicas_count &&
+ least_total_replicas_count > total_replicas_count) {
+ least_total_replicas_count = total_replicas_count;
+ selected = dn.get();
+ }
}
}
-
- LOG_INFO(
- "{}: put pid({}) to dir({}), which has {} replicas of current app, {} replicas totally",
- dsn_primary_address(),
- pid,
- selected->tag,
- least_app_replicas_count,
- least_total_replicas_count);
-
- selected->holding_replicas[pid.get_app_id()].emplace(pid);
- dir = utils::filesystem::path_combine(selected->full_dir, buffer);
+ if (selected) {
Review Comment:
```suggestion
if (selected != nullptr) {
```
##########
src/common/test/fs_manager_test.cpp:
##########
@@ -64,19 +102,113 @@ TEST(fs_manager, dir_update_disk_status)
TEST(fs_manager, get_dir_node)
{
fs_manager fm;
- fm.initialize({"/data1"}, {"data1"});
+ fm.initialize({"./data1"}, {"data1"});
+ const auto &dns = fm.get_dir_nodes();
+ ASSERT_EQ(1, dns.size());
+ const auto &base_dir =
+ dns[0]->full_dir.substr(0, dns[0]->full_dir.size() - std::string("/data1").size());
ASSERT_EQ(nullptr, fm.get_dir_node(""));
ASSERT_EQ(nullptr, fm.get_dir_node("/"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1/"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1/replica1"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1/"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1/replica1"));
+
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2"));
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2/"));
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2/replica1"));
+}
+
+TEST(fs_manager, find_replica_dir)
+{
+ fs_manager fm;
+ fm.initialize({"./data1", "./data2", "./data3"}, {"data1", "data2", "data3"});
+
+ const char *app_type = "find_replica_dir";
+ gpid test_pid(1, 0);
+
+ // Clear up the remaining directories if exist.
+ for (const auto &dn : fm.get_dir_nodes()) {
+ remove_path(dn->replica_dir(app_type, test_pid));
+ }
+
+ ASSERT_EQ(nullptr, fm.find_replica_dir(app_type, test_pid));
+ dir_node *dn = fm.create_replica_dir_if_necessary(app_type, test_pid);
+ ASSERT_NE(nullptr, dn);
+ const auto dir = dn->replica_dir(app_type, test_pid);
+ ASSERT_TRUE(directory_exists(dir));
+ dir_node *dn1 = fm.find_replica_dir(app_type, test_pid);
Review Comment:
```suggestion
auto dn1 = fm.find_replica_dir(app_type, test_pid);
```
##########
src/replica/test/replica_disk_migrate_test.cpp:
##########
@@ -362,31 +362,44 @@ TEST_F(replica_disk_migrate_test, disk_migrate_replica_update)
}
}
+// Test load from new replica dir failed, then fall back to load from origin dir succeed,
+// and then mark the "new" replica dir as ".gar".
TEST_F(replica_disk_migrate_test, disk_migrate_replica_open)
{
+ gpid test_pid(app_info_1.app_id, 4);
+
+ // Suppose gpid 1.4 is migrated from tag_2 to tag_empty_1.
auto &request = *fake_migrate_rpc.mutable_request();
- request.pid = dsn::gpid(app_info_1.app_id, 4);
+ request.pid = test_pid;
request.origin_disk = "tag_2";
request.target_disk = "tag_empty_1";
- remove_mock_dir_node(request.origin_disk);
+ // Remove the gpid 1.4 dir which is created in constructor.
+ const std::string kReplicaOriginDir =
+ fmt::format("./{}/{}.replica", request.origin_disk, request.pid.to_string());
+ utils::filesystem::remove_path(kReplicaOriginDir);
+ stub->get_fs_manager()->remove_replica(test_pid);
+
+ // Create the related dirs.
const std::string kReplicaOriginSuffixDir = fmt::format(
Review Comment:
```suggestion
const auto kReplicaOriginSuffixDir = fmt::format(
```
##########
src/replica/test/replica_disk_migrate_test.cpp:
##########
@@ -362,31 +362,44 @@ TEST_F(replica_disk_migrate_test, disk_migrate_replica_update)
}
}
+// Test load from new replica dir failed, then fall back to load from origin dir succeed,
+// and then mark the "new" replica dir as ".gar".
TEST_F(replica_disk_migrate_test, disk_migrate_replica_open)
{
+ gpid test_pid(app_info_1.app_id, 4);
+
+ // Suppose gpid 1.4 is migrated from tag_2 to tag_empty_1.
auto &request = *fake_migrate_rpc.mutable_request();
- request.pid = dsn::gpid(app_info_1.app_id, 4);
+ request.pid = test_pid;
request.origin_disk = "tag_2";
request.target_disk = "tag_empty_1";
- remove_mock_dir_node(request.origin_disk);
+ // Remove the gpid 1.4 dir which is created in constructor.
+ const std::string kReplicaOriginDir =
+ fmt::format("./{}/{}.replica", request.origin_disk, request.pid.to_string());
+ utils::filesystem::remove_path(kReplicaOriginDir);
+ stub->get_fs_manager()->remove_replica(test_pid);
+
+ // Create the related dirs.
const std::string kReplicaOriginSuffixDir = fmt::format(
"./{}/{}.replica.disk.migrate.ori/", request.origin_disk, request.pid.to_string());
const std::string kReplicaNewDir =
fmt::format("./{}/{}.replica/", request.target_disk, request.pid.to_string());
utils::filesystem::create_directory(kReplicaOriginSuffixDir);
utils::filesystem::create_directory(kReplicaNewDir);
+ // The replica can be opened nomally. In fact, the original dir is opened, and the new dir will
+ // be garbage.
fail::cfg("mock_replica_load", "return()");
- const std::string kReplicaOriginDir =
- fmt::format("./{}/{}.replica", request.origin_disk, request.pid.to_string());
- const std::string kReplicaGarDir =
- fmt::format("./{}/{}.replica.gar", request.target_disk, request.pid.to_string());
open_replica(app_info_1, request.pid);
+ // Check it works as expected.
+ const std::string kReplicaGarDir =
+ fmt::format("./{}/{}.replica.gar", request.target_disk, request.pid.to_string());
Review Comment:
```suggestion
fmt::format("./{}/{}.replica.gar", request.target_disk, request.pid);
```
##########
src/replica/test/replica_disk_migrate_test.cpp:
##########
@@ -362,31 +362,44 @@ TEST_F(replica_disk_migrate_test, disk_migrate_replica_update)
}
}
+// Test load from new replica dir failed, then fall back to load from origin dir succeed,
+// and then mark the "new" replica dir as ".gar".
TEST_F(replica_disk_migrate_test, disk_migrate_replica_open)
{
+ gpid test_pid(app_info_1.app_id, 4);
+
+ // Suppose gpid 1.4 is migrated from tag_2 to tag_empty_1.
auto &request = *fake_migrate_rpc.mutable_request();
- request.pid = dsn::gpid(app_info_1.app_id, 4);
+ request.pid = test_pid;
request.origin_disk = "tag_2";
request.target_disk = "tag_empty_1";
- remove_mock_dir_node(request.origin_disk);
+ // Remove the gpid 1.4 dir which is created in constructor.
+ const std::string kReplicaOriginDir =
+ fmt::format("./{}/{}.replica", request.origin_disk, request.pid.to_string());
+ utils::filesystem::remove_path(kReplicaOriginDir);
+ stub->get_fs_manager()->remove_replica(test_pid);
+
+ // Create the related dirs.
const std::string kReplicaOriginSuffixDir = fmt::format(
"./{}/{}.replica.disk.migrate.ori/", request.origin_disk, request.pid.to_string());
const std::string kReplicaNewDir =
Review Comment:
```suggestion
const auto kReplicaNewDir =
```
##########
src/common/test/fs_manager_test.cpp:
##########
@@ -64,19 +102,113 @@ TEST(fs_manager, dir_update_disk_status)
TEST(fs_manager, get_dir_node)
{
fs_manager fm;
- fm.initialize({"/data1"}, {"data1"});
+ fm.initialize({"./data1"}, {"data1"});
+ const auto &dns = fm.get_dir_nodes();
+ ASSERT_EQ(1, dns.size());
+ const auto &base_dir =
+ dns[0]->full_dir.substr(0, dns[0]->full_dir.size() - std::string("/data1").size());
ASSERT_EQ(nullptr, fm.get_dir_node(""));
ASSERT_EQ(nullptr, fm.get_dir_node("/"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1/"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1/replica1"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1/"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1/replica1"));
+
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2"));
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2/"));
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2/replica1"));
+}
+
+TEST(fs_manager, find_replica_dir)
+{
+ fs_manager fm;
+ fm.initialize({"./data1", "./data2", "./data3"}, {"data1", "data2", "data3"});
+
+ const char *app_type = "find_replica_dir";
+ gpid test_pid(1, 0);
+
+ // Clear up the remaining directories if exist.
+ for (const auto &dn : fm.get_dir_nodes()) {
+ remove_path(dn->replica_dir(app_type, test_pid));
+ }
+
+ ASSERT_EQ(nullptr, fm.find_replica_dir(app_type, test_pid));
+ dir_node *dn = fm.create_replica_dir_if_necessary(app_type, test_pid);
Review Comment:
```suggestion
auto dn = fm.create_replica_dir_if_necessary(app_type, test_pid);
```
##########
src/replica/test/replica_disk_migrate_test.cpp:
##########
@@ -362,31 +362,44 @@ TEST_F(replica_disk_migrate_test, disk_migrate_replica_update)
}
}
+// Test load from new replica dir failed, then fall back to load from origin dir succeed,
+// and then mark the "new" replica dir as ".gar".
TEST_F(replica_disk_migrate_test, disk_migrate_replica_open)
{
+ gpid test_pid(app_info_1.app_id, 4);
+
+ // Suppose gpid 1.4 is migrated from tag_2 to tag_empty_1.
auto &request = *fake_migrate_rpc.mutable_request();
- request.pid = dsn::gpid(app_info_1.app_id, 4);
+ request.pid = test_pid;
request.origin_disk = "tag_2";
request.target_disk = "tag_empty_1";
- remove_mock_dir_node(request.origin_disk);
+ // Remove the gpid 1.4 dir which is created in constructor.
+ const std::string kReplicaOriginDir =
+ fmt::format("./{}/{}.replica", request.origin_disk, request.pid.to_string());
+ utils::filesystem::remove_path(kReplicaOriginDir);
+ stub->get_fs_manager()->remove_replica(test_pid);
+
+ // Create the related dirs.
const std::string kReplicaOriginSuffixDir = fmt::format(
"./{}/{}.replica.disk.migrate.ori/", request.origin_disk, request.pid.to_string());
const std::string kReplicaNewDir =
fmt::format("./{}/{}.replica/", request.target_disk, request.pid.to_string());
Review Comment:
```suggestion
fmt::format("./{}/{}.replica/", request.target_disk, request.pid);
```
##########
src/replica/test/replica_disk_migrate_test.cpp:
##########
@@ -362,31 +362,44 @@ TEST_F(replica_disk_migrate_test, disk_migrate_replica_update)
}
}
+// Test load from new replica dir failed, then fall back to load from origin dir succeed,
+// and then mark the "new" replica dir as ".gar".
TEST_F(replica_disk_migrate_test, disk_migrate_replica_open)
{
+ gpid test_pid(app_info_1.app_id, 4);
+
+ // Suppose gpid 1.4 is migrated from tag_2 to tag_empty_1.
auto &request = *fake_migrate_rpc.mutable_request();
- request.pid = dsn::gpid(app_info_1.app_id, 4);
+ request.pid = test_pid;
request.origin_disk = "tag_2";
request.target_disk = "tag_empty_1";
- remove_mock_dir_node(request.origin_disk);
+ // Remove the gpid 1.4 dir which is created in constructor.
+ const std::string kReplicaOriginDir =
+ fmt::format("./{}/{}.replica", request.origin_disk, request.pid.to_string());
+ utils::filesystem::remove_path(kReplicaOriginDir);
+ stub->get_fs_manager()->remove_replica(test_pid);
+
+ // Create the related dirs.
const std::string kReplicaOriginSuffixDir = fmt::format(
"./{}/{}.replica.disk.migrate.ori/", request.origin_disk, request.pid.to_string());
Review Comment:
```suggestion
"./{}/{}.replica.disk.migrate.ori/", request.origin_disk, request.pid);
```
##########
src/replica/test/replica_disk_migrate_test.cpp:
##########
@@ -362,31 +362,44 @@ TEST_F(replica_disk_migrate_test, disk_migrate_replica_update)
}
}
+// Test load from new replica dir failed, then fall back to load from origin dir succeed,
+// and then mark the "new" replica dir as ".gar".
TEST_F(replica_disk_migrate_test, disk_migrate_replica_open)
{
+ gpid test_pid(app_info_1.app_id, 4);
+
+ // Suppose gpid 1.4 is migrated from tag_2 to tag_empty_1.
auto &request = *fake_migrate_rpc.mutable_request();
- request.pid = dsn::gpid(app_info_1.app_id, 4);
+ request.pid = test_pid;
request.origin_disk = "tag_2";
request.target_disk = "tag_empty_1";
- remove_mock_dir_node(request.origin_disk);
+ // Remove the gpid 1.4 dir which is created in constructor.
+ const std::string kReplicaOriginDir =
Review Comment:
```suggestion
const auto kReplicaOriginDir =
```
##########
src/replica/replica_stub.cpp:
##########
@@ -1768,36 +1738,38 @@ void replica_stub::init_gc_for_test()
void replica_stub::on_gc_replica(replica_stub_ptr this_, gpid id)
{
- std::string replica_path;
std::pair<app_info, replica_info> closed_info;
-
{
zauto_write_lock l(_replicas_lock);
auto iter = _closed_replicas.find(id);
if (iter == _closed_replicas.end())
return;
closed_info = iter->second;
_closed_replicas.erase(iter);
- _fs_manager.remove_replica(id);
}
+ _fs_manager.remove_replica(id);
- replica_path = get_replica_dir(closed_info.first.app_type.c_str(), id, false);
- if (replica_path.empty()) {
+ const auto dn = _fs_manager.find_replica_dir(closed_info.first.app_type.c_str(), id);
Review Comment:
```suggestion
const auto * const dn = _fs_manager.find_replica_dir(closed_info.first.app_type.c_str(), id);
```
##########
src/common/test/fs_manager_test.cpp:
##########
@@ -64,19 +102,113 @@ TEST(fs_manager, dir_update_disk_status)
TEST(fs_manager, get_dir_node)
{
fs_manager fm;
- fm.initialize({"/data1"}, {"data1"});
+ fm.initialize({"./data1"}, {"data1"});
+ const auto &dns = fm.get_dir_nodes();
+ ASSERT_EQ(1, dns.size());
+ const auto &base_dir =
+ dns[0]->full_dir.substr(0, dns[0]->full_dir.size() - std::string("/data1").size());
ASSERT_EQ(nullptr, fm.get_dir_node(""));
ASSERT_EQ(nullptr, fm.get_dir_node("/"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1/"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1/replica1"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1/"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1/replica1"));
+
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2"));
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2/"));
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2/replica1"));
+}
+
+TEST(fs_manager, find_replica_dir)
+{
+ fs_manager fm;
+ fm.initialize({"./data1", "./data2", "./data3"}, {"data1", "data2", "data3"});
+
+ const char *app_type = "find_replica_dir";
+ gpid test_pid(1, 0);
+
+ // Clear up the remaining directories if exist.
+ for (const auto &dn : fm.get_dir_nodes()) {
+ remove_path(dn->replica_dir(app_type, test_pid));
+ }
+
+ ASSERT_EQ(nullptr, fm.find_replica_dir(app_type, test_pid));
+ dir_node *dn = fm.create_replica_dir_if_necessary(app_type, test_pid);
+ ASSERT_NE(nullptr, dn);
+ const auto dir = dn->replica_dir(app_type, test_pid);
+ ASSERT_TRUE(directory_exists(dir));
+ dir_node *dn1 = fm.find_replica_dir(app_type, test_pid);
+ ASSERT_EQ(dn, dn1);
+}
+
+TEST(fs_manager, create_replica_dir_if_necessary)
+{
+ fs_manager fm;
+
+ const char *app_type = "create_replica_dir_if_necessary";
+ gpid test_pid(1, 0);
- ASSERT_EQ(nullptr, fm.get_dir_node("/data2"));
- ASSERT_EQ(nullptr, fm.get_dir_node("/data2/"));
- ASSERT_EQ(nullptr, fm.get_dir_node("/data2/replica1"));
+ // Could not find a valid dir_node.
+ ASSERT_EQ(nullptr, fm.create_replica_dir_if_necessary(app_type, test_pid));
+
+ // It's able to create a dir for the replica after a valid dir_node has been added.
+ fm.add_new_dir_node("./data1", "data1");
+ dir_node *dn = fm.create_replica_dir_if_necessary(app_type, test_pid);
+ ASSERT_NE(nullptr, dn);
+ ASSERT_EQ("data1", dn->tag);
}
+TEST(fs_manager, create_child_replica_dir)
+{
+ fs_manager fm;
+ fm.initialize({"./data1", "./data2", "./data3"}, {"data1", "data2", "data3"});
+
+ const char *app_type = "create_child_replica_dir";
+ gpid test_pid(1, 0);
+ gpid test_child_pid(1, 0);
+
+ dir_node *dn = fm.create_replica_dir_if_necessary(app_type, test_pid);
+ ASSERT_NE(nullptr, dn);
+ const auto dir = dn->replica_dir(app_type, test_pid);
+
+ dir_node *child_dn = fm.create_child_replica_dir(app_type, test_child_pid, dir);
Review Comment:
```suggestion
auto child_dn = fm.create_child_replica_dir(app_type, test_child_pid, dir);
```
##########
src/common/test/fs_manager_test.cpp:
##########
@@ -64,19 +102,113 @@ TEST(fs_manager, dir_update_disk_status)
TEST(fs_manager, get_dir_node)
{
fs_manager fm;
- fm.initialize({"/data1"}, {"data1"});
+ fm.initialize({"./data1"}, {"data1"});
+ const auto &dns = fm.get_dir_nodes();
+ ASSERT_EQ(1, dns.size());
+ const auto &base_dir =
+ dns[0]->full_dir.substr(0, dns[0]->full_dir.size() - std::string("/data1").size());
ASSERT_EQ(nullptr, fm.get_dir_node(""));
ASSERT_EQ(nullptr, fm.get_dir_node("/"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1/"));
- ASSERT_NE(nullptr, fm.get_dir_node("/data1/replica1"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1/"));
+ ASSERT_NE(nullptr, fm.get_dir_node(base_dir + "/data1/replica1"));
+
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2"));
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2/"));
+ ASSERT_EQ(nullptr, fm.get_dir_node(base_dir + "/data2/replica1"));
+}
+
+TEST(fs_manager, find_replica_dir)
+{
+ fs_manager fm;
+ fm.initialize({"./data1", "./data2", "./data3"}, {"data1", "data2", "data3"});
+
+ const char *app_type = "find_replica_dir";
+ gpid test_pid(1, 0);
+
+ // Clear up the remaining directories if exist.
+ for (const auto &dn : fm.get_dir_nodes()) {
+ remove_path(dn->replica_dir(app_type, test_pid));
+ }
+
+ ASSERT_EQ(nullptr, fm.find_replica_dir(app_type, test_pid));
+ dir_node *dn = fm.create_replica_dir_if_necessary(app_type, test_pid);
+ ASSERT_NE(nullptr, dn);
+ const auto dir = dn->replica_dir(app_type, test_pid);
+ ASSERT_TRUE(directory_exists(dir));
+ dir_node *dn1 = fm.find_replica_dir(app_type, test_pid);
+ ASSERT_EQ(dn, dn1);
+}
+
+TEST(fs_manager, create_replica_dir_if_necessary)
+{
+ fs_manager fm;
+
+ const char *app_type = "create_replica_dir_if_necessary";
+ gpid test_pid(1, 0);
- ASSERT_EQ(nullptr, fm.get_dir_node("/data2"));
- ASSERT_EQ(nullptr, fm.get_dir_node("/data2/"));
- ASSERT_EQ(nullptr, fm.get_dir_node("/data2/replica1"));
+ // Could not find a valid dir_node.
+ ASSERT_EQ(nullptr, fm.create_replica_dir_if_necessary(app_type, test_pid));
+
+ // It's able to create a dir for the replica after a valid dir_node has been added.
+ fm.add_new_dir_node("./data1", "data1");
+ dir_node *dn = fm.create_replica_dir_if_necessary(app_type, test_pid);
+ ASSERT_NE(nullptr, dn);
+ ASSERT_EQ("data1", dn->tag);
}
+TEST(fs_manager, create_child_replica_dir)
+{
+ fs_manager fm;
+ fm.initialize({"./data1", "./data2", "./data3"}, {"data1", "data2", "data3"});
+
+ const char *app_type = "create_child_replica_dir";
+ gpid test_pid(1, 0);
+ gpid test_child_pid(1, 0);
+
+ dir_node *dn = fm.create_replica_dir_if_necessary(app_type, test_pid);
+ ASSERT_NE(nullptr, dn);
+ const auto dir = dn->replica_dir(app_type, test_pid);
+
+ dir_node *child_dn = fm.create_child_replica_dir(app_type, test_child_pid, dir);
+ ASSERT_EQ(dn, child_dn);
+ const auto child_dir = child_dn->replica_dir(app_type, test_child_pid);
+ ASSERT_TRUE(directory_exists(child_dir));
+ ASSERT_EQ(dir, child_dir);
+}
+
+TEST(fs_manager, find_best_dir_for_new_replica)
+{
+ // dn1 | 1.0, 1.1 +1.6
+ // dn2 | 1.2, 1.3 +1.7 2.0
+ // dn3 | 1.4 +1.5 +1.7 2.1
+ auto dn1 = std::make_shared<dir_node>("./data1", "data1");
+ dn1->holding_replicas[1] = {gpid(1, 0), gpid(1, 1)};
+ auto dn2 = std::make_shared<dir_node>("./data2", "data2");
+ dn2->holding_replicas[1] = {gpid(1, 2), gpid(1, 3)};
+ dn2->holding_replicas[2] = {gpid(2, 0)};
+ auto dn3 = std::make_shared<dir_node>("./data3", "data3");
+ dn3->holding_replicas[1] = {gpid(1, 4)};
+ dn3->holding_replicas[2] = {gpid(2, 1)};
+ fs_manager fm;
+ fm._dir_nodes = {dn1, dn2, dn3};
+
+ gpid pid_1_5(1, 5);
+ dir_node *dn = fm.find_best_dir_for_new_replica(pid_1_5);
Review Comment:
```suggestion
auto dn = fm.find_best_dir_for_new_replica(pid_1_5);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 merged pull request #1477: refactor: improve the single-responsibility of class fs_manager
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 merged PR #1477:
URL: https://github.com/apache/incubator-pegasus/pull/1477
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] WHBANG commented on pull request #1477: refactor: improve the single-responsibility of class fs_manager
Posted by "WHBANG (via GitHub)" <gi...@apache.org>.
WHBANG commented on PR #1477:
URL: https://github.com/apache/incubator-pegasus/pull/1477#issuecomment-1559267105
LGT1. (^_^)v
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] levy5307 commented on a diff in pull request #1477: refactor: improve the single-responsibility of class fs_manager
Posted by "levy5307 (via GitHub)" <gi...@apache.org>.
levy5307 commented on code in PR #1477:
URL: https://github.com/apache/incubator-pegasus/pull/1477#discussion_r1201886296
##########
src/common/fs_manager.cpp:
##########
@@ -80,6 +87,11 @@ uint64_t dir_node::replicas_count(app_id id) const
return iter->second.size();
}
+std::string dir_node::replica_dir(const char *app_type, const dsn::gpid &pid) const
Review Comment:
Could you change the type of `app_type` to `string_view`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] levy5307 commented on a diff in pull request #1477: refactor: improve the single-responsibility of class fs_manager
Posted by "levy5307 (via GitHub)" <gi...@apache.org>.
levy5307 commented on code in PR #1477:
URL: https://github.com/apache/incubator-pegasus/pull/1477#discussion_r1201888318
##########
src/common/fs_manager.cpp:
##########
@@ -359,5 +395,77 @@ bool fs_manager::is_dir_node_available(const std::string &data_dir, const std::s
return false;
}
+dir_node *fs_manager::find_replica_dir(const char *app_type, gpid pid)
+{
+ std::string replica_dir;
+ dir_node *replica_dn = nullptr;
+ {
+ zauto_read_lock l(_lock);
+ for (const auto &dn : _dir_nodes) {
+ const auto dir = dn->replica_dir(app_type, pid);
+ if (utils::filesystem::directory_exists(dir)) {
+ // Check if there are duplicate replica instance directories.
+ CHECK(replica_dir.empty(), "replica dir conflict: {} <--> {}", dir, replica_dir);
+ replica_dir = dir;
+ replica_dn = dn.get();
+ }
+ }
+ }
+
+ return replica_dn;
+}
+
+dir_node *fs_manager::create_replica_dir_if_necessary(const char *app_type, gpid pid)
+{
+ // Try to find the replica directory.
+ dir_node *replica_dn = find_replica_dir(app_type, pid);
Review Comment:
auto replica_dn
##########
src/replica/disk_cleaner.cpp:
##########
@@ -67,14 +68,14 @@ const std::string kFolderSuffixBak = ".bak";
const std::string kFolderSuffixOri = ".ori";
const std::string kFolderSuffixTmp = ".tmp";
-error_s disk_remove_useless_dirs(const std::vector<std::string> &data_dirs,
+error_s disk_remove_useless_dirs(const std::vector<std::shared_ptr<dir_node>> &dir_nodes,
Review Comment:
What about using `weak_ptr` instead of shared_ptr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1477: refactor: improve the single-responsibility of class fs_manager
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1477:
URL: https://github.com/apache/incubator-pegasus/pull/1477#discussion_r1202182238
##########
src/replica/disk_cleaner.cpp:
##########
@@ -67,14 +68,14 @@ const std::string kFolderSuffixBak = ".bak";
const std::string kFolderSuffixOri = ".ori";
const std::string kFolderSuffixTmp = ".tmp";
-error_s disk_remove_useless_dirs(const std::vector<std::string> &data_dirs,
+error_s disk_remove_useless_dirs(const std::vector<std::shared_ptr<dir_node>> &dir_nodes,
Review Comment:
I don't think it's necessary. If supporting to remove dir_node from fs manager, we have to make sure it's safe to access the dir_node in disk_remove_useless_dirs() even if it has been removed, std::shared_ptr is safe in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org