Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: improve the single-responsibility of class fs_manager #1477

Merged
merged 5 commits into from
May 25, 2023

Conversation

acelyc111
Copy link
Member

@acelyc111 acelyc111 commented May 17, 2023

#1383

This patch moves some functions to fs_manager which are more reasonable to be
responsibilities of class fs_manager rather than those of class replica_stub.

A new configuration is added:

[replication]
+ ignore_broken_disk = true

@github-actions github-actions bot added the cpp label May 17, 2023
@acelyc111 acelyc111 force-pushed the mv_func_2_fs_mgr2 branch 2 times, most recently from f187ca9 to ae54a7a Compare May 18, 2023 07:16
@acelyc111 acelyc111 marked this pull request as ready for review May 18, 2023 07:26
@acelyc111 acelyc111 changed the title refactor: improve the single-responsibility of class fs_manager refactor: improve the single-responsibility of class fs_manager WIP May 19, 2023
@github-actions github-actions bot added scripts and removed scripts labels May 22, 2023
@acelyc111 acelyc111 changed the title refactor: improve the single-responsibility of class fs_manager WIP refactor: improve the single-responsibility of class fs_manager May 23, 2023
src/common/fs_manager.cpp Outdated Show resolved Hide resolved
src/common/fs_manager.cpp Outdated Show resolved Hide resolved
@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using weak_ptr instead of shared_ptr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@WHBANG
Copy link
Contributor

WHBANG commented May 23, 2023

LGT1. (^_^)v

levy5307
levy5307 previously approved these changes May 24, 2023
src/replica/test/mock_utils.h Outdated Show resolved Hide resolved
src/replica/test/mock_utils.h Outdated Show resolved Hide resolved
src/replica/test/replica_disk_migrate_test.cpp Outdated Show resolved Hide resolved
src/replica/test/replica_disk_migrate_test.cpp Outdated Show resolved Hide resolved
src/replica/test/replica_disk_migrate_test.cpp Outdated Show resolved Hide resolved
src/replica/replica_stub.cpp Outdated Show resolved Hide resolved
src/common/test/fs_manager_test.cpp Outdated Show resolved Hide resolved
src/common/test/fs_manager_test.cpp Outdated Show resolved Hide resolved
src/common/test/fs_manager_test.cpp Outdated Show resolved Hide resolved
src/common/test/fs_manager_test.cpp Outdated Show resolved Hide resolved
@acelyc111 acelyc111 merged commit d5f2259 into apache:master May 25, 2023
@empiredan empiredan mentioned this pull request Aug 21, 2023
@empiredan empiredan added the type/config-change Added or modified configuration that should be noted on release note of new version. label Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp type/config-change Added or modified configuration that should be noted on release note of new version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants