You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "levy5307 (via GitHub)" <gi...@apache.org> on 2023/05/17 08:04:35 UTC
[GitHub] [incubator-pegasus] levy5307 commented on a diff in pull request #1476: refactor: minor refactor on class fs_manager
levy5307 commented on code in PR #1476:
URL: https://github.com/apache/incubator-pegasus/pull/1476#discussion_r1196096433
##########
src/common/fs_manager.cpp:
##########
@@ -134,68 +137,63 @@ bool dir_node::update_disk_stat(const bool update_disk_status)
return (old_status != new_status);
}
-fs_manager::fs_manager(bool for_test)
+fs_manager::fs_manager()
{
- if (!for_test) {
- _counter_total_capacity_mb.init_app_counter("eon.replica_stub",
- "disk.capacity.total(MB)",
+ _counter_total_capacity_mb.init_app_counter("eon.replica_stub",
+ "disk.capacity.total(MB)",
+ COUNTER_TYPE_NUMBER,
+ "total disk capacity in MB");
+ _counter_total_available_mb.init_app_counter("eon.replica_stub",
+ "disk.available.total(MB)",
+ COUNTER_TYPE_NUMBER,
+ "total disk available in MB");
+ _counter_total_available_ratio.init_app_counter("eon.replica_stub",
+ "disk.available.total.ratio",
COUNTER_TYPE_NUMBER,
- "total disk capacity in MB");
- _counter_total_available_mb.init_app_counter("eon.replica_stub",
- "disk.available.total(MB)",
- COUNTER_TYPE_NUMBER,
- "total disk available in MB");
- _counter_total_available_ratio.init_app_counter("eon.replica_stub",
- "disk.available.total.ratio",
- COUNTER_TYPE_NUMBER,
- "total disk available ratio");
- _counter_min_available_ratio.init_app_counter("eon.replica_stub",
- "disk.available.min.ratio",
- COUNTER_TYPE_NUMBER,
- "minimal disk available ratio in all disks");
- _counter_max_available_ratio.init_app_counter("eon.replica_stub",
- "disk.available.max.ratio",
- COUNTER_TYPE_NUMBER,
- "maximal disk available ratio in all disks");
- }
+ "total disk available ratio");
+ _counter_min_available_ratio.init_app_counter("eon.replica_stub",
+ "disk.available.min.ratio",
+ COUNTER_TYPE_NUMBER,
+ "minimal disk available ratio in all disks");
+ _counter_max_available_ratio.init_app_counter("eon.replica_stub",
+ "disk.available.max.ratio",
+ COUNTER_TYPE_NUMBER,
+ "maximal disk available ratio in all disks");
}
dir_node *fs_manager::get_dir_node(const std::string &subdir) const
{
- zauto_read_lock l(_lock);
std::string norm_subdir;
utils::filesystem::get_normalized_path(subdir, norm_subdir);
- for (auto &n : _dir_nodes) {
- // if input is a subdir of some dir_nodes
- const std::string &d = n->full_dir;
- if (norm_subdir.compare(0, d.size(), d) == 0 &&
- (norm_subdir.size() == d.size() || norm_subdir[d.size()] == '/')) {
- return n.get();
+
+ zauto_read_lock l(_lock);
+ for (const auto &dn : _dir_nodes) {
+ // Check if 'subdir' is a sub-directory of 'dn'.
+ const std::string &dir = dn->full_dir;
+ if (norm_subdir.compare(0, dir.size(), dir) == 0 &&
Review Comment:
``` if (norm_subdir.compare(0, dir.size(), dir) == 0 &&
(norm_subdir.size() == dir.size() || norm_subdir[dir.size()] == '/'))```
==>
```
if ((norm_subdir.size() == dir.size() || norm_subdir[dir.size()] == '/') && norm_subdir.compare(0, dir.size(), dir) == 0)
```
##########
src/common/fs_manager.cpp:
##########
@@ -211,20 +209,21 @@ dsn::error_code fs_manager::get_disk_tag(const std::string &dir, std::string &ta
void fs_manager::add_replica(const gpid &pid, const std::string &pid_dir)
{
- dir_node *n = get_dir_node(pid_dir);
- if (nullptr == n) {
+ const auto &dn = get_dir_node(pid_dir);
+ if (nullptr == dn) {
LOG_ERROR(
"{}: dir({}) of gpid({}) haven't registered", dsn_primary_address(), pid_dir, pid);
+ return;
+ }
+
+ zauto_write_lock l(_lock);
+ auto &replicas_for_app = dn->holding_replicas[pid.get_app_id()];
Review Comment:
```
{
zauto_write_lock l(_lock);
auto &replicas_for_app = dn->holding_replicas[pid.get_app_id()];
auto result = replicas_for_app.emplace(pid);
}
```
##########
src/common/fs_manager.cpp:
##########
@@ -211,20 +209,21 @@ dsn::error_code fs_manager::get_disk_tag(const std::string &dir, std::string &ta
void fs_manager::add_replica(const gpid &pid, const std::string &pid_dir)
{
- dir_node *n = get_dir_node(pid_dir);
- if (nullptr == n) {
+ const auto &dn = get_dir_node(pid_dir);
+ if (nullptr == dn) {
Review Comment:
`dsn_unlikely` ?
--
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