From 2abd68a46fdf43c3a36e3f85910cf27c07c5e696 Mon Sep 17 00:00:00 2001 From: HeYuchen <377710264@qq.com> Date: Sat, 26 Dec 2020 16:03:36 +0800 Subject: [PATCH] fix: block service download file return error when file exists (#703) --- src/block_service/block_service_manager.cpp | 38 ++++--------------- src/block_service/block_service_manager.h | 1 + .../test/block_service_manager_test.cpp | 11 +++--- src/replica/bulk_load/replica_bulk_loader.cpp | 13 +++++-- src/replica/replica_restore.cpp | 12 ++++-- 5 files changed, 30 insertions(+), 45 deletions(-) diff --git a/src/block_service/block_service_manager.cpp b/src/block_service/block_service_manager.cpp index 3408306e2b..9b842526e9 100644 --- a/src/block_service/block_service_manager.cpp +++ b/src/block_service/block_service_manager.cpp @@ -118,6 +118,13 @@ error_code block_service_manager::download_file(const std::string &remote_dir, block_filesystem *fs, /*out*/ uint64_t &download_file_size) { + // local file exists + const std::string local_file_name = utils::filesystem::path_combine(local_dir, file_name); + if (utils::filesystem::file_exists(local_file_name)) { + ddebug_f("local file({}) exists", local_file_name); + return ERR_PATH_ALREADY_EXIST; + } + task_tracker tracker; // Create a block_file object. @@ -131,37 +138,6 @@ error_code block_service_manager::download_file(const std::string &remote_dir, } block_file_ptr bf = create_resp.file_handle; - const std::string local_file_name = utils::filesystem::path_combine(local_dir, file_name); - if (utils::filesystem::file_exists(local_file_name)) { - std::string current_md5; - error_code e = utils::filesystem::md5sum(local_file_name, current_md5); - // local file exists - if (e == ERR_OK && current_md5 == bf->get_md5sum()) { - download_file_size = bf->get_size(); - ddebug_f("local file({}) has been downloaded, file_size = {}", - local_file_name, - download_file_size); - return ERR_OK; - } - // local file has same file name with remote file, but there are different - // remove local file and download it from remote file provider - if (e != ERR_OK) { - dwarn_f("calculate file({}) md5 failed, should remove and redownload it", - local_file_name); - } else { - dwarn_f("local file({}) is different from remote file({}), md5: local({}) VS " - "remote({}), should remove and redownload it", - local_file_name, - bf->file_name(), - current_md5, - bf->get_md5sum()); - } - if (!utils::filesystem::remove_path(local_file_name)) { - derror_f("failed to remove file({})", local_file_name); - return ERR_FILE_OPERATION_FAILED; - } - } - download_response resp = download_block_file_sync(local_file_name, bf.get(), &tracker); if (resp.err != ERR_OK) { // during bulk load process, ERR_OBJECT_NOT_FOUND will be considered as a recoverable diff --git a/src/block_service/block_service_manager.h b/src/block_service/block_service_manager.h index 3ac64ab2ab..82d59effa6 100644 --- a/src/block_service/block_service_manager.h +++ b/src/block_service/block_service_manager.h @@ -33,6 +33,7 @@ class block_service_manager // \return ERR_FILE_OPERATION_FAILED: local file system error // \return ERR_FS_INTERNAL: remote file system error // \return ERR_CORRUPTION: file not exist or damaged + // \return ERR_PATH_ALREADY_EXIST: local file exist // if download file succeed, download_err = ERR_OK and set download_file_size // // TODO(wutao1): create block_filesystem_wrapper instead. diff --git a/src/block_service/test/block_service_manager_test.cpp b/src/block_service/test/block_service_manager_test.cpp index fce53ac1b3..13db9060e6 100644 --- a/src/block_service/test/block_service_manager_test.cpp +++ b/src/block_service/test/block_service_manager_test.cpp @@ -75,15 +75,14 @@ TEST_F(block_service_manager_test, do_download_remote_file_not_exist) ASSERT_EQ(err, ERR_CORRUPTION); // file does not exist } -TEST_F(block_service_manager_test, do_download_redownload_file) +TEST_F(block_service_manager_test, do_download_same_name_file) { // local file exists, but md5 not matched with remote file - // expected to remove old local file and redownload it create_local_file(FILE_NAME); create_remote_file(FILE_NAME, 2333, "md5_not_match"); uint64_t download_size = 0; - ASSERT_EQ(test_download_file(download_size), ERR_OK); - ASSERT_EQ(download_size, 2333); + ASSERT_EQ(test_download_file(download_size), ERR_PATH_ALREADY_EXIST); + ASSERT_EQ(download_size, 0); } TEST_F(block_service_manager_test, do_download_file_exist) @@ -91,8 +90,8 @@ TEST_F(block_service_manager_test, do_download_file_exist) create_local_file(FILE_NAME); create_remote_file(FILE_NAME, _file_meta.size, _file_meta.md5); uint64_t download_size = 0; - ASSERT_EQ(test_download_file(download_size), ERR_OK); - ASSERT_EQ(download_size, _file_meta.size); + ASSERT_EQ(test_download_file(download_size), ERR_PATH_ALREADY_EXIST); + ASSERT_EQ(download_size, 0); } TEST_F(block_service_manager_test, do_download_succeed) diff --git a/src/replica/bulk_load/replica_bulk_loader.cpp b/src/replica/bulk_load/replica_bulk_loader.cpp index 5e36b65292..91e31f41a5 100644 --- a/src/replica/bulk_load/replica_bulk_loader.cpp +++ b/src/replica/bulk_load/replica_bulk_loader.cpp @@ -391,7 +391,7 @@ error_code replica_bulk_loader::download_sst_files(const std::string &remote_dir uint64_t file_size = 0; error_code err = _stub->_block_service_manager.download_file( remote_dir, local_dir, bulk_load_constant::BULK_LOAD_METADATA, fs, file_size); - if (err != ERR_OK) { + if (err != ERR_OK && err != ERR_PATH_ALREADY_EXIST) { derror_replica("download bulk load metadata file failed, error = {}", err.to_string()); return err; } @@ -414,9 +414,14 @@ error_code replica_bulk_loader::download_sst_files(const std::string &remote_dir remote_dir, local_dir, f_meta.name, fs, f_size); const std::string &file_name = utils::filesystem::path_combine(local_dir, f_meta.name); - if (ec == ERR_OK && - !utils::filesystem::verify_file(file_name, f_meta.md5, f_meta.size)) { - ec = ERR_CORRUPTION; + if (ec == ERR_OK || ec == ERR_PATH_ALREADY_EXIST) { + if (!utils::filesystem::verify_file(file_name, f_meta.md5, f_meta.size)) { + ec = ERR_CORRUPTION; + } else if (ec == ERR_PATH_ALREADY_EXIST) { + // local file exist and is verified + ec = ERR_OK; + f_size = f_meta.size; + } } if (ec != ERR_OK) { try_decrease_bulk_load_download_count(); diff --git a/src/replica/replica_restore.cpp b/src/replica/replica_restore.cpp index 55e9b3b200..4eb17e52a0 100644 --- a/src/replica/replica_restore.cpp +++ b/src/replica/replica_restore.cpp @@ -120,9 +120,13 @@ error_code replica::download_checkpoint(const configuration_restore_request &req remote_chkpt_dir, local_chkpt_dir, f_meta.name, fs, f_size); const std::string file_name = utils::filesystem::path_combine(local_chkpt_dir, f_meta.name); - if (download_err == ERR_OK && - !utils::filesystem::verify_file(file_name, f_meta.md5, f_meta.size)) { - download_err = ERR_CORRUPTION; + if (download_err == ERR_OK || download_err == ERR_PATH_ALREADY_EXIST) { + if (!utils::filesystem::verify_file(file_name, f_meta.md5, f_meta.size)) { + download_err = ERR_CORRUPTION; + } else if (download_err == ERR_PATH_ALREADY_EXIST) { + download_err = ERR_OK; + f_size = f_meta.size; + } } if (download_err != ERR_OK) { @@ -167,7 +171,7 @@ error_code replica::get_backup_metadata(block_filesystem *fs, cold_backup_constant::BACKUP_METADATA, fs, download_file_size); - if (err != ERR_OK) { + if (err != ERR_OK && err != ERR_PATH_ALREADY_EXIST) { derror_replica("download backup_metadata failed, file({}), reason({})", utils::filesystem::path_combine(remote_chkpt_dir, cold_backup_constant::BACKUP_METADATA),