From b551e2627eb13fcd8757ec9c4c70ff937de7657d Mon Sep 17 00:00:00 2001 From: Yingchun Lai Date: Tue, 10 Oct 2023 14:07:49 +0800 Subject: [PATCH] refactor: some minor refactors without functional changes (#1629) --- .licenserc.yaml | 2 +- src/block_service/block_service.h | 6 +- src/geo/bench/bench.cpp | 6 +- src/meta/test/main.cpp | 6 +- ...{nfs_node_impl.cpp => nfs_node_simple.cpp} | 13 ++- src/nfs/nfs_node_simple.h | 14 ++- src/nfs/nfs_server_impl.cpp | 102 ++++++++---------- src/nfs/nfs_server_impl.h | 12 +-- src/server/test/pegasus_server_impl_test.cpp | 25 ++--- src/test/function_test/base_api/test_copy.cpp | 12 +-- 10 files changed, 104 insertions(+), 94 deletions(-) rename src/nfs/{nfs_node_impl.cpp => nfs_node_simple.cpp} (93%) diff --git a/.licenserc.yaml b/.licenserc.yaml index c6f63afd8b..53d316d23b 100644 --- a/.licenserc.yaml +++ b/.licenserc.yaml @@ -312,7 +312,7 @@ header: - 'src/nfs/nfs_client_impl.h' - 'src/nfs/nfs_code_definition.h' - 'src/nfs/nfs_node.cpp' - - 'src/nfs/nfs_node_impl.cpp' + - 'src/nfs/nfs_node_simple.cpp' - 'src/nfs/nfs_node_simple.h' - 'src/nfs/nfs_server_impl.cpp' - 'src/nfs/nfs_server_impl.h' diff --git a/src/block_service/block_service.h b/src/block_service/block_service.h index 96f1416445..d351dcf44a 100644 --- a/src/block_service/block_service.h +++ b/src/block_service/block_service.h @@ -238,8 +238,8 @@ struct upload_request */ struct upload_response { - dsn::error_code err; - uint64_t uploaded_size; + dsn::error_code err = ERR_OK; + uint64_t uploaded_size = 0; }; typedef std::function upload_callback; typedef future_task upload_future; @@ -378,6 +378,8 @@ class block_file : public dsn::ref_counter const write_callback &cb, dsn::task_tracker *tracker = nullptr) = 0; + // TODO(yingchun): it seems every read() will read the whole file, consider to read the whole + // file directly. /** * @brief read * @param req, ref {@link #read_request} diff --git a/src/geo/bench/bench.cpp b/src/geo/bench/bench.cpp index e65714b7ad..0ac170b02e 100644 --- a/src/geo/bench/bench.cpp +++ b/src/geo/bench/bench.cpp @@ -76,10 +76,12 @@ int main(int argc, char **argv) } } + // TODO(yingchun): the benchmark can not exit normally, we need to fix it later. pegasus::geo::geo_client my_geo( "config.ini", cluster_name.c_str(), app_name.c_str(), geo_app_name.c_str()); - if (!my_geo.set_max_level(max_level).is_ok()) { - std::cerr << "set_max_level failed" << std::endl; + auto err = my_geo.set_max_level(max_level); + if (!err.is_ok()) { + std::cerr << "set_max_level failed, err: " << err << std::endl; return -1; } diff --git a/src/meta/test/main.cpp b/src/meta/test/main.cpp index fd82dd1779..e410572bcc 100644 --- a/src/meta/test/main.cpp +++ b/src/meta/test/main.cpp @@ -63,7 +63,11 @@ TEST(meta, state_sync) { g_app->state_sync_test(); } TEST(meta, update_configuration) { g_app->update_configuration_test(); } -TEST(meta, balancer_validator) { g_app->balancer_validator(); } +TEST(meta, balancer_validator) +{ + // TODO(yingchun): this test last too long time, optimize it! + g_app->balancer_validator(); +} TEST(meta, apply_balancer) { g_app->apply_balancer_test(); } diff --git a/src/nfs/nfs_node_impl.cpp b/src/nfs/nfs_node_simple.cpp similarity index 93% rename from src/nfs/nfs_node_impl.cpp rename to src/nfs/nfs_node_simple.cpp index 387547fd88..bb334a5085 100644 --- a/src/nfs/nfs_node_impl.cpp +++ b/src/nfs/nfs_node_simple.cpp @@ -80,12 +80,17 @@ void nfs_node_simple::register_async_rpc_handler_for_test() error_code nfs_node_simple::stop() { - delete _server; - _server = nullptr; + if (_server != nullptr) { + _server->close_service(); - delete _client; - _client = nullptr; + delete _server; + _server = nullptr; + } + if (_client != nullptr) { + delete _client; + _client = nullptr; + } return ERR_OK; } diff --git a/src/nfs/nfs_node_simple.h b/src/nfs/nfs_node_simple.h index 2376b1e34e..15e2344168 100644 --- a/src/nfs/nfs_node_simple.h +++ b/src/nfs/nfs_node_simple.h @@ -34,14 +34,24 @@ */ #pragma once -#include "runtime/tool_api.h" +#include + #include "nfs/nfs_node.h" +#include "utils/error_code.h" namespace dsn { +class aio_task; +template +class rpc_replier; + namespace service { -class nfs_service_impl; +class copy_request; +class copy_response; +class get_file_size_request; +class get_file_size_response; class nfs_client_impl; +class nfs_service_impl; class nfs_node_simple : public nfs_node { diff --git a/src/nfs/nfs_server_impl.cpp b/src/nfs/nfs_server_impl.cpp index cadacba1a3..08821042a4 100644 --- a/src/nfs/nfs_server_impl.cpp +++ b/src/nfs/nfs_server_impl.cpp @@ -26,12 +26,11 @@ #include "nfs/nfs_server_impl.h" -#include #include -#include #include #include #include +#include #include #include "nfs/nfs_code_definition.h" @@ -39,10 +38,10 @@ #include "runtime/api_layer1.h" #include "runtime/task/async_calls.h" #include "utils/TokenBucket.h" +#include "utils/env.h" #include "utils/filesystem.h" #include "utils/flags.h" #include "utils/ports.h" -#include "utils/safe_strerror_posix.h" #include "utils/string_conv.h" #include "utils/utils.h" @@ -90,38 +89,35 @@ void nfs_service_impl::on_copy(const ::dsn::service::copy_request &request, dsn::utils::filesystem::path_combine(request.source_dir, request.file_name); disk_file *dfile = nullptr; - { + do { zauto_lock l(_handles_map_lock); auto it = _handles_map.find(file_path); // find file handle cache first - if (it == _handles_map.end()) { dfile = file::open(file_path.c_str(), O_RDONLY | O_BINARY, 0); - if (dfile != nullptr) { - auto fh = std::make_shared(); - fh->file_handle = dfile; - fh->file_access_count = 1; - fh->last_access_time = dsn_now_ms(); - _handles_map.insert(std::make_pair(file_path, std::move(fh))); + if (dfile == nullptr) { + LOG_ERROR("[nfs_service] open file {} failed", file_path); + ::dsn::service::copy_response resp; + resp.error = ERR_OBJECT_NOT_FOUND; + reply(resp); + return; } - } else { - dfile = it->second->file_handle; - it->second->file_access_count++; - it->second->last_access_time = dsn_now_ms(); - } - } - - LOG_DEBUG( - "nfs: copy file {} [{}, {}]", file_path, request.offset, request.offset + request.size); - if (dfile == nullptr) { - LOG_ERROR("[nfs_service] open file {} failed", file_path); - ::dsn::service::copy_response resp; - resp.error = ERR_OBJECT_NOT_FOUND; - reply(resp); - return; - } - - std::shared_ptr cp = std::make_shared(std::move(reply)); + auto fh = std::make_shared(); + fh->file_handle = dfile; + it = _handles_map.insert(std::make_pair(file_path, std::move(fh))).first; + } + dfile = it->second->file_handle; + it->second->file_access_count++; + it->second->last_access_time = dsn_now_ms(); + } while (false); + + CHECK_NOTNULL(dfile, ""); + LOG_DEBUG("nfs: copy from file {} [{}, {}]", + file_path, + request.offset, + request.offset + request.size); + + auto cp = std::make_shared(std::move(reply)); cp->bb = blob(dsn::utils::make_shared_array(request.size), request.size); cp->dst_dir = request.dst_dir; cp->source_disk_tag = request.source_disk_tag; @@ -182,58 +178,53 @@ void nfs_service_impl::on_get_file_size( { get_file_size_response resp; error_code err = ERR_OK; - std::vector file_list; std::string folder = request.source_dir; + // TODO(yingchun): refactor the following code! if (request.file_list.size() == 0) // return all file size in the destination file folder { if (!dsn::utils::filesystem::directory_exists(folder)) { LOG_ERROR("[nfs_service] directory {} not exist", folder); err = ERR_OBJECT_NOT_FOUND; } else { + std::vector file_list; if (!dsn::utils::filesystem::get_subfiles(folder, file_list, true)) { LOG_ERROR("[nfs_service] get subfiles of directory {} failed", folder); err = ERR_FILE_OPERATION_FAILED; } else { - for (auto &fpath : file_list) { - // TODO: using uint64 instead as file ma - // Done + for (const auto &fpath : file_list) { int64_t sz; - if (!dsn::utils::filesystem::file_size(fpath, sz)) { + // TODO(yingchun): check if there are any files that are not sensitive (not + // encrypted). + if (!dsn::utils::filesystem::file_size( + fpath, dsn::utils::FileDataType::kSensitive, sz)) { LOG_ERROR("[nfs_service] get size of file {} failed", fpath); err = ERR_FILE_OPERATION_FAILED; break; } - resp.size_list.push_back((uint64_t)sz); + resp.size_list.push_back(sz); resp.file_list.push_back( fpath.substr(request.source_dir.length(), fpath.length() - 1)); } - file_list.clear(); } } } else // return file size in the request file folder { - for (size_t i = 0; i < request.file_list.size(); i++) { - std::string file_path = - dsn::utils::filesystem::path_combine(folder, request.file_list[i]); - - struct stat st; - if (0 != ::stat(file_path.c_str(), &st)) { - LOG_ERROR("[nfs_service] get stat of file {} failed, err = {}", - file_path, - dsn::utils::safe_strerror(errno)); - err = ERR_OBJECT_NOT_FOUND; + for (const auto &file_name : request.file_list) { + std::string file_path = dsn::utils::filesystem::path_combine(folder, file_name); + int64_t sz; + // TODO(yingchun): check if there are any files that are not sensitive (not encrypted). + if (!dsn::utils::filesystem::file_size( + file_path, dsn::utils::FileDataType::kSensitive, sz)) { + LOG_ERROR("[nfs_service] get size of file {} failed", file_path); + err = ERR_FILE_OPERATION_FAILED; break; } - // TODO: using int64 instead as file may exceed the size of 32bit - // Done - uint64_t size = st.st_size; - - resp.size_list.push_back(size); - resp.file_list.push_back((folder + request.file_list[i]) - .substr(request.source_dir.length(), - (folder + request.file_list[i]).length() - 1)); + resp.size_list.push_back(sz); + resp.file_list.push_back( + (folder + file_name) + .substr(request.source_dir.length(), (folder + file_name).length() - 1)); } } @@ -253,8 +244,9 @@ void nfs_service_impl::close_file() // release out-of-date file handle dsn_now_ms() - fptr->last_access_time > (uint64_t)FLAGS_file_close_expire_time_ms) { LOG_DEBUG("nfs: close file handle {}", it->first); it = _handles_map.erase(it); - } else + } else { it++; + } } } diff --git a/src/nfs/nfs_server_impl.h b/src/nfs/nfs_server_impl.h index 9ba1134040..ece68ecb33 100644 --- a/src/nfs/nfs_server_impl.h +++ b/src/nfs/nfs_server_impl.h @@ -66,7 +66,6 @@ class nfs_service_impl : public ::dsn::serverlet void register_cli_commands(); - // TODO(yingchun): seems nobody call it, can be removed? void close_service() { unregister_rpc_handler(RPC_NFS_COPY); @@ -107,14 +106,9 @@ class nfs_service_impl : public ::dsn::serverlet struct file_handle_info_on_server { - disk_file *file_handle; - int32_t file_access_count; // concurrent r/w count - uint64_t last_access_time; // last touch time - - file_handle_info_on_server() - : file_handle(nullptr), file_access_count(0), last_access_time(0) - { - } + disk_file *file_handle = nullptr; + int32_t file_access_count = 0; // concurrent r/w count + uint64_t last_access_time = 0; // last touch time ~file_handle_info_on_server() { diff --git a/src/server/test/pegasus_server_impl_test.cpp b/src/server/test/pegasus_server_impl_test.cpp index 9776571ae9..718446821d 100644 --- a/src/server/test/pegasus_server_impl_test.cpp +++ b/src/server/test/pegasus_server_impl_test.cpp @@ -21,6 +21,7 @@ #include #include #include +// IWYU pragma: no_include // IWYU pragma: no_include // IWYU pragma: no_include #include @@ -124,10 +125,10 @@ class pegasus_server_impl_test : public pegasus_server_test_base } } - start(all_test_envs); + ASSERT_EQ(dsn::ERR_OK, start(all_test_envs)); if (is_restart) { - _server->stop(false); - start(); + ASSERT_EQ(dsn::ERR_OK, _server->stop(false)); + ASSERT_EQ(dsn::ERR_OK, start()); } std::map query_envs; @@ -145,20 +146,20 @@ class pegasus_server_impl_test : public pegasus_server_test_base TEST_F(pegasus_server_impl_test, test_table_level_slow_query) { - start(); + ASSERT_EQ(dsn::ERR_OK, start()); test_table_level_slow_query(); } TEST_F(pegasus_server_impl_test, default_data_version) { - start(); + ASSERT_EQ(dsn::ERR_OK, start()); ASSERT_EQ(_server->_pegasus_data_version, 1); } TEST_F(pegasus_server_impl_test, test_open_db_with_latest_options) { // open a new db with no app env. - start(); + ASSERT_EQ(dsn::ERR_OK, start()); ASSERT_EQ(ROCKSDB_ENV_USAGE_SCENARIO_NORMAL, _server->_usage_scenario); // set bulk_load scenario for the db. ASSERT_TRUE(_server->set_usage_scenario(ROCKSDB_ENV_USAGE_SCENARIO_BULK_LOAD)); @@ -167,8 +168,8 @@ TEST_F(pegasus_server_impl_test, test_open_db_with_latest_options) ASSERT_EQ(1000000000, opts.level0_file_num_compaction_trigger); ASSERT_EQ(true, opts.disable_auto_compactions); // reopen the db. - _server->stop(false); - start(); + ASSERT_EQ(dsn::ERR_OK, _server->stop(false)); + ASSERT_EQ(dsn::ERR_OK, start()); ASSERT_EQ(ROCKSDB_ENV_USAGE_SCENARIO_BULK_LOAD, _server->_usage_scenario); ASSERT_EQ(opts.level0_file_num_compaction_trigger, _server->_db->GetOptions().level0_file_num_compaction_trigger); @@ -179,7 +180,7 @@ TEST_F(pegasus_server_impl_test, test_open_db_with_app_envs) { std::map envs; envs[ROCKSDB_ENV_USAGE_SCENARIO_KEY] = ROCKSDB_ENV_USAGE_SCENARIO_BULK_LOAD; - start(envs); + ASSERT_EQ(dsn::ERR_OK, start(envs)); ASSERT_EQ(ROCKSDB_ENV_USAGE_SCENARIO_BULK_LOAD, _server->_usage_scenario); } @@ -197,16 +198,16 @@ TEST_F(pegasus_server_impl_test, test_restart_db_with_rocksdb_envs) TEST_F(pegasus_server_impl_test, test_stop_db_twice) { - start(); + ASSERT_EQ(dsn::ERR_OK, start()); ASSERT_TRUE(_server->_is_open); ASSERT_TRUE(_server->_db != nullptr); - _server->stop(false); + ASSERT_EQ(dsn::ERR_OK, _server->stop(false)); ASSERT_FALSE(_server->_is_open); ASSERT_TRUE(_server->_db == nullptr); // stop again - _server->stop(false); + ASSERT_EQ(dsn::ERR_OK, _server->stop(false)); ASSERT_FALSE(_server->_is_open); ASSERT_TRUE(_server->_db == nullptr); } diff --git a/src/test/function_test/base_api/test_copy.cpp b/src/test/function_test/base_api/test_copy.cpp index 910a07fff6..d2155f12f0 100644 --- a/src/test/function_test/base_api/test_copy.cpp +++ b/src/test/function_test/base_api/test_copy.cpp @@ -98,9 +98,9 @@ class copy_data_test : public test_util ASSERT_EQ(dsn::ERR_OK, ddl_client_->create_app( destination_app_name, "pegasus", default_partitions, 3, {}, false)); - srouce_client_ = + source_client_ = pegasus_client_factory::get_client(cluster_name_.c_str(), source_app_name.c_str()); - ASSERT_NE(nullptr, srouce_client_); + ASSERT_NE(nullptr, source_client_); destination_client_ = pegasus_client_factory::get_client(cluster_name_.c_str(), destination_app_name.c_str()); ASSERT_NE(nullptr, destination_client_); @@ -132,7 +132,7 @@ class copy_data_test : public test_util while (expect_data_[empty_hash_key].size() < 1000) { sort_key = random_string(); value = random_string(); - ASSERT_EQ(PERR_OK, srouce_client_->set(empty_hash_key, sort_key, value)) + ASSERT_EQ(PERR_OK, source_client_->set(empty_hash_key, sort_key, value)) << "hash_key=" << hash_key << ", sort_key=" << sort_key; expect_data_[empty_hash_key][sort_key] = value; } @@ -142,7 +142,7 @@ class copy_data_test : public test_util while (expect_data_[hash_key].size() < 10) { sort_key = random_string(); value = random_string(); - ASSERT_EQ(PERR_OK, srouce_client_->set(hash_key, sort_key, value)) + ASSERT_EQ(PERR_OK, source_client_->set(hash_key, sort_key, value)) << "hash_key=" << hash_key << ", sort_key=" << sort_key; expect_data_[hash_key][sort_key] = value; } @@ -163,7 +163,7 @@ class copy_data_test : public test_util char buffer_[256]; map> expect_data_; - pegasus_client *srouce_client_; + pegasus_client *source_client_; pegasus_client *destination_client_; }; const char copy_data_test::CCH[] = @@ -176,7 +176,7 @@ TEST_F(copy_data_test, EMPTY_HASH_KEY_COPY) pegasus_client::scan_options options; options.return_expire_ts = true; vector raw_scanners; - ASSERT_EQ(PERR_OK, srouce_client_->get_unordered_scanners(INT_MAX, options, raw_scanners)); + ASSERT_EQ(PERR_OK, source_client_->get_unordered_scanners(INT_MAX, options, raw_scanners)); LOG_INFO("open source app scanner succeed, partition_count = {}", raw_scanners.size());