Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

Commit

Permalink
fix: block service download file return error when file exists (#703)
Browse files Browse the repository at this point in the history
  • Loading branch information
@hycdong
hycdong authored Dec 26, 2020
1 parent 00e3d98 commit 2abd68a
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 45 deletions.
38 changes: 7 additions & 31 deletions src/block_service/block_service_manager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/block_service/block_service_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 5 additions & 6 deletions src/block_service/test/block_service_manager_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,24 +75,23 @@ 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)
{
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)
Expand Down
13 changes: 9 additions & 4 deletions src/replica/bulk_load/replica_bulk_loader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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();
Expand Down
12 changes: 8 additions & 4 deletions src/replica/replica_restore.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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),
Expand Down

0 comments on commit 2abd68a

Please sign in to comment.