empiredan commented on code in PR #1935:
URL:
https://github.com/apache/incubator-pegasus/pull/1935#discussion_r1516029583
##########
src/replica/replica_stub.cpp:
##########
@@ -1904,57 +1904,73 @@ replica *replica_stub::new_replica(gpid gpid,
return rep;
}
-replica *replica_stub::load_replica(dir_node *dn, const char *dir)
+bool replica_stub::validate_replica_dir(const std::string &dir,
+ app_info &ai,
+ gpid &pid,
+ std::string &hint_message)
{
- FAIL_POINT_INJECT_F("mock_replica_load",
- [&](absl::string_view) -> replica * { return nullptr;
});
+ if (!utils::filesystem::directory_exists(dir)) {
+ hint_message = fmt::format("replica dir '{}' not exist", dir);
+ return false;
+ }
char splitters[] = {'\\', '/', 0};
- std::string name = utils::get_last_component(std::string(dir), splitters);
+ const auto name = utils::get_last_component(dir, splitters);
if (name.empty()) {
- LOG_ERROR("invalid replica dir {}", dir);
- return nullptr;
+ hint_message = fmt::format("invalid replica dir '{}'", dir);
+ return false;
}
- char app_type[128];
+ char app_type[128] = {0};
int32_t app_id, pidx;
if (3 != sscanf(name.c_str(), "%d.%d.%s", &app_id, &pidx, app_type)) {
- LOG_ERROR("invalid replica dir {}", dir);
- return nullptr;
+ hint_message = fmt::format("invalid replica dir '{}'", dir);
+ return false;
}
- gpid pid(app_id, pidx);
- if (!utils::filesystem::directory_exists(dir)) {
- LOG_ERROR("replica dir {} not exist", dir);
- return nullptr;
+ pid = gpid(app_id, pidx);
+ replica_app_info rai(&ai);
+ const auto ai_path = utils::filesystem::path_combine(dir,
replica::kAppInfo);
Review Comment:
```suggestion
const auto &ai_path = utils::filesystem::path_combine(dir,
replica_app_info::kAppInfo);
```
##########
src/replica/replica_stub.cpp:
##########
@@ -1900,57 +1900,73 @@ replica *replica_stub::new_replica(gpid gpid,
return rep;
}
-replica *replica_stub::load_replica(dir_node *dn, const char *dir)
+bool replica_stub::validate_replica_dir(const std::string &dir,
+ app_info &ai,
+ gpid &pid,
+ std::string &hint_message)
{
- FAIL_POINT_INJECT_F("mock_replica_load",
- [&](absl::string_view) -> replica * { return nullptr;
});
+ if (!utils::filesystem::directory_exists(dir)) {
+ hint_message = fmt::format("replica dir '{}' not exist", dir);
+ return false;
+ }
char splitters[] = {'\\', '/', 0};
- std::string name = utils::get_last_component(std::string(dir), splitters);
+ const auto name = utils::get_last_component(dir, splitters);
if (name.empty()) {
- LOG_ERROR("invalid replica dir {}", dir);
- return nullptr;
+ hint_message = fmt::format("invalid replica dir '{}'", dir);
+ return false;
}
- char app_type[128];
+ char app_type[128] = {0};
int32_t app_id, pidx;
if (3 != sscanf(name.c_str(), "%d.%d.%s", &app_id, &pidx, app_type)) {
- LOG_ERROR("invalid replica dir {}", dir);
- return nullptr;
+ hint_message = fmt::format("invalid replica dir '{}'", dir);
+ return false;
}
- gpid pid(app_id, pidx);
- if (!utils::filesystem::directory_exists(dir)) {
- LOG_ERROR("replica dir {} not exist", dir);
- return nullptr;
+ pid = gpid(app_id, pidx);
+ replica_app_info rai(&ai);
+ const auto ai_path = utils::filesystem::path_combine(dir,
replica::kAppInfo);
+ auto err = rai.load(ai_path);
+ if (ERR_OK != err) {
+ hint_message = fmt::format("load app-info from '{}' failed, err = {}",
ai_path, err);
+ return false;
}
- dsn::app_info info;
- replica_app_info info2(&info);
- std::string path = utils::filesystem::path_combine(dir, replica::kAppInfo);
- auto err = info2.load(path);
- if (ERR_OK != err) {
- LOG_ERROR("load app-info from {} failed, err = {}", path, err);
- return nullptr;
+ if (ai.app_type != app_type) {
+ hint_message = fmt::format("unmatched app type '{}' for '{}'",
ai.app_type, ai_path);
+ return false;
}
- if (info.app_type != app_type) {
- LOG_ERROR("unmatched app type {} for {}", info.app_type, path);
- return nullptr;
+ if (ai.partition_count < pidx) {
+ hint_message = fmt::format(
+ "partition[{}], count={}, this replica may be partition split
garbage partition, "
+ "ignore it",
+ pid,
+ ai.partition_count);
+ return false;
}
- if (info.partition_count < pidx) {
- LOG_ERROR("partition[{}], count={}, this replica may be partition
split garbage partition, "
- "ignore it",
- pid,
- info.partition_count);
+ return true;
+}
+
+replica *replica_stub::load_replica(dir_node *dn, const char *dir)
+{
+ FAIL_POINT_INJECT_F("mock_replica_load",
+ [&](absl::string_view) -> replica * { return nullptr;
});
+
+ app_info ai;
+ gpid pid;
+ std::string hint_message;
+ if (!validate_replica_dir(dir, ai, pid, hint_message)) {
+ LOG_ERROR("invalid replica dir '{}', hint: {}", dir, hint_message);
return nullptr;
}
- // The replica's directory must exists when creating a replica.
- CHECK_EQ(dir, dn->replica_dir(app_type, pid));
- auto *rep = new replica(this, pid, info, dn, false);
- err = rep->initialize_on_load();
+ // The replica's directory must exist when creating a replica.
+ CHECK_EQ(dir, dn->replica_dir(ai.app_type, pid));
+ auto *rep = new replica(this, pid, ai, dn, false);
+ auto err = rep->initialize_on_load();
Review Comment:
```suggestion
const auto &err = rep->initialize_on_load();
```
##########
src/replica/replica_stub.cpp:
##########
@@ -1904,57 +1904,73 @@ replica *replica_stub::new_replica(gpid gpid,
return rep;
}
-replica *replica_stub::load_replica(dir_node *dn, const char *dir)
+bool replica_stub::validate_replica_dir(const std::string &dir,
+ app_info &ai,
+ gpid &pid,
+ std::string &hint_message)
{
- FAIL_POINT_INJECT_F("mock_replica_load",
- [&](absl::string_view) -> replica * { return nullptr;
});
+ if (!utils::filesystem::directory_exists(dir)) {
+ hint_message = fmt::format("replica dir '{}' not exist", dir);
+ return false;
+ }
char splitters[] = {'\\', '/', 0};
- std::string name = utils::get_last_component(std::string(dir), splitters);
+ const auto name = utils::get_last_component(dir, splitters);
if (name.empty()) {
- LOG_ERROR("invalid replica dir {}", dir);
- return nullptr;
+ hint_message = fmt::format("invalid replica dir '{}'", dir);
+ return false;
}
- char app_type[128];
+ char app_type[128] = {0};
int32_t app_id, pidx;
if (3 != sscanf(name.c_str(), "%d.%d.%s", &app_id, &pidx, app_type)) {
- LOG_ERROR("invalid replica dir {}", dir);
- return nullptr;
+ hint_message = fmt::format("invalid replica dir '{}'", dir);
+ return false;
}
- gpid pid(app_id, pidx);
- if (!utils::filesystem::directory_exists(dir)) {
- LOG_ERROR("replica dir {} not exist", dir);
- return nullptr;
+ pid = gpid(app_id, pidx);
+ replica_app_info rai(&ai);
+ const auto ai_path = utils::filesystem::path_combine(dir,
replica::kAppInfo);
+ auto err = rai.load(ai_path);
Review Comment:
```suggestion
const auto &err = rai.load(ai_path);
```
##########
src/replica/replica_stub.cpp:
##########
@@ -1904,57 +1904,73 @@ replica *replica_stub::new_replica(gpid gpid,
return rep;
}
-replica *replica_stub::load_replica(dir_node *dn, const char *dir)
+bool replica_stub::validate_replica_dir(const std::string &dir,
+ app_info &ai,
+ gpid &pid,
+ std::string &hint_message)
{
- FAIL_POINT_INJECT_F("mock_replica_load",
- [&](absl::string_view) -> replica * { return nullptr;
});
+ if (!utils::filesystem::directory_exists(dir)) {
+ hint_message = fmt::format("replica dir '{}' not exist", dir);
+ return false;
+ }
char splitters[] = {'\\', '/', 0};
- std::string name = utils::get_last_component(std::string(dir), splitters);
+ const auto name = utils::get_last_component(dir, splitters);
if (name.empty()) {
- LOG_ERROR("invalid replica dir {}", dir);
- return nullptr;
+ hint_message = fmt::format("invalid replica dir '{}'", dir);
+ return false;
}
- char app_type[128];
+ char app_type[128] = {0};
int32_t app_id, pidx;
if (3 != sscanf(name.c_str(), "%d.%d.%s", &app_id, &pidx, app_type)) {
- LOG_ERROR("invalid replica dir {}", dir);
- return nullptr;
+ hint_message = fmt::format("invalid replica dir '{}'", dir);
+ return false;
}
- gpid pid(app_id, pidx);
- if (!utils::filesystem::directory_exists(dir)) {
- LOG_ERROR("replica dir {} not exist", dir);
- return nullptr;
+ pid = gpid(app_id, pidx);
+ replica_app_info rai(&ai);
+ const auto ai_path = utils::filesystem::path_combine(dir,
replica::kAppInfo);
+ auto err = rai.load(ai_path);
+ if (ERR_OK != err) {
+ hint_message = fmt::format("load app-info from '{}' failed, err = {}",
ai_path, err);
+ return false;
}
- dsn::app_info info;
- replica_app_info info2(&info);
- std::string path = utils::filesystem::path_combine(dir,
replica_app_info::kAppInfo);
- auto err = info2.load(path);
- if (ERR_OK != err) {
- LOG_ERROR("load app-info from {} failed, err = {}", path, err);
- return nullptr;
+ if (ai.app_type != app_type) {
+ hint_message = fmt::format("unmatched app type '{}' for '{}'",
ai.app_type, ai_path);
+ return false;
}
- if (info.app_type != app_type) {
- LOG_ERROR("unmatched app type {} for {}", info.app_type, path);
- return nullptr;
+ if (ai.partition_count < pidx) {
Review Comment:
Might be `ai.partition_count <= pidx` ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
