From 40f7ddb6cc00c7f80fa6a88994066dfdbef3ee17 Mon Sep 17 00:00:00 2001 From: kssenii Date: Mon, 18 Apr 2022 15:07:07 +0200 Subject: [PATCH 1/4] Fix --- src/Disks/IDiskRemote.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Disks/IDiskRemote.cpp b/src/Disks/IDiskRemote.cpp index ead951084ad..2c1f230fe64 100644 --- a/src/Disks/IDiskRemote.cpp +++ b/src/Disks/IDiskRemote.cpp @@ -27,6 +27,7 @@ namespace ErrorCodes extern const int FILE_DOESNT_EXIST; extern const int BAD_FILE_TYPE; extern const int MEMORY_LIMIT_EXCEEDED; + extern const int FILE_DOESNT_EXIST; } @@ -359,7 +360,16 @@ void IDiskRemote::getRemotePathsRecursive(const String & local_path, std::vector { if (metadata_disk->isFile(local_path)) { - paths_map.emplace_back(local_path, getRemotePaths(local_path)); + try + { + paths_map.emplace_back(local_path, getRemotePaths(local_path)); + } + catch (const Exception & e) + { + if (e.code() == ErrorCodes::FILE_DOESNT_EXIST) + return; + throw; + } } else { From f7a2fa487d1f1b756bfd03eb7f928dd7a4556d27 Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Tue, 19 Apr 2022 10:45:48 +0200 Subject: [PATCH 2/4] Fix style check --- src/Disks/IDiskRemote.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Disks/IDiskRemote.cpp b/src/Disks/IDiskRemote.cpp index 2c1f230fe64..e5d32131ef8 100644 --- a/src/Disks/IDiskRemote.cpp +++ b/src/Disks/IDiskRemote.cpp @@ -27,7 +27,6 @@ namespace ErrorCodes extern const int FILE_DOESNT_EXIST; extern const int BAD_FILE_TYPE; extern const int MEMORY_LIMIT_EXCEEDED; - extern const int FILE_DOESNT_EXIST; } From 2fb331bcec06aa07eaeb6127b905bf828daa5178 Mon Sep 17 00:00:00 2001 From: kssenii Date: Tue, 19 Apr 2022 17:32:46 +0200 Subject: [PATCH 3/4] Better --- src/Disks/DiskLocal.cpp | 22 +++++++++++++++++----- src/Disks/IDiskRemote.cpp | 3 +++ 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/Disks/DiskLocal.cpp b/src/Disks/DiskLocal.cpp index d81782a8af1..cbeec1004e0 100644 --- a/src/Disks/DiskLocal.cpp +++ b/src/Disks/DiskLocal.cpp @@ -173,8 +173,8 @@ class DiskLocalDirectoryIterator final : public IDiskDirectoryIterator { public: DiskLocalDirectoryIterator() = default; - DiskLocalDirectoryIterator(const String & disk_path_, const String & dir_path_) - : dir_path(dir_path_), entry(fs::path(disk_path_) / dir_path_) + DiskLocalDirectoryIterator(fs::directory_iterator && entry_, const String & dir_path_) + : dir_path(dir_path_), entry(std::move(entry_)) { } @@ -319,9 +319,21 @@ DiskDirectoryIteratorPtr DiskLocal::iterateDirectory(const String & path) { fs::path meta_path = fs::path(disk_path) / path; if (!broken && fs::exists(meta_path) && fs::is_directory(meta_path)) - return std::make_unique(disk_path, path); - else - return std::make_unique(); + { + try + { + fs::directory_iterator entry(fs::path(disk_path) / path); + return std::make_unique(std::move(entry), path); + } + catch (const fs::filesystem_error & e) + { + if (e.code() == std::errc::no_such_file_or_directory) + return std::make_unique(); + throw; + } + } + + return std::make_unique(); } void DiskLocal::moveFile(const String & from_path, const String & to_path) diff --git a/src/Disks/IDiskRemote.cpp b/src/Disks/IDiskRemote.cpp index e5d32131ef8..4ea5fe1bb19 100644 --- a/src/Disks/IDiskRemote.cpp +++ b/src/Disks/IDiskRemote.cpp @@ -359,6 +359,7 @@ void IDiskRemote::getRemotePathsRecursive(const String & local_path, std::vector { if (metadata_disk->isFile(local_path)) { + /// File could be concurrently deleted (for example because of a merge). try { paths_map.emplace_back(local_path, getRemotePaths(local_path)); @@ -372,6 +373,8 @@ void IDiskRemote::getRemotePathsRecursive(const String & local_path, std::vector } else { + /// Metadata disk is currently only DislLocal. DiskLocal::iterateDirectory will return empty directory iterator + /// if file does not exist (even on cocnurrent file delition), for (auto it = iterateDirectory(local_path); it->isValid(); it->next()) IDiskRemote::getRemotePathsRecursive(fs::path(local_path) / it->name(), paths_map); } From 5fc4aaaa2dfbfbef677a8587159fb42c512b362c Mon Sep 17 00:00:00 2001 From: kssenii Date: Wed, 20 Apr 2022 10:42:07 +0200 Subject: [PATCH 4/4] Better --- src/Disks/DiskLocal.cpp | 22 +++++----------------- src/Disks/IDiskRemote.cpp | 18 ++++++++++++++---- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/Disks/DiskLocal.cpp b/src/Disks/DiskLocal.cpp index cbeec1004e0..d81782a8af1 100644 --- a/src/Disks/DiskLocal.cpp +++ b/src/Disks/DiskLocal.cpp @@ -173,8 +173,8 @@ class DiskLocalDirectoryIterator final : public IDiskDirectoryIterator { public: DiskLocalDirectoryIterator() = default; - DiskLocalDirectoryIterator(fs::directory_iterator && entry_, const String & dir_path_) - : dir_path(dir_path_), entry(std::move(entry_)) + DiskLocalDirectoryIterator(const String & disk_path_, const String & dir_path_) + : dir_path(dir_path_), entry(fs::path(disk_path_) / dir_path_) { } @@ -319,21 +319,9 @@ DiskDirectoryIteratorPtr DiskLocal::iterateDirectory(const String & path) { fs::path meta_path = fs::path(disk_path) / path; if (!broken && fs::exists(meta_path) && fs::is_directory(meta_path)) - { - try - { - fs::directory_iterator entry(fs::path(disk_path) / path); - return std::make_unique(std::move(entry), path); - } - catch (const fs::filesystem_error & e) - { - if (e.code() == std::errc::no_such_file_or_directory) - return std::make_unique(); - throw; - } - } - - return std::make_unique(); + return std::make_unique(disk_path, path); + else + return std::make_unique(); } void DiskLocal::moveFile(const String & from_path, const String & to_path) diff --git a/src/Disks/IDiskRemote.cpp b/src/Disks/IDiskRemote.cpp index 4ea5fe1bb19..45448177081 100644 --- a/src/Disks/IDiskRemote.cpp +++ b/src/Disks/IDiskRemote.cpp @@ -357,9 +357,9 @@ std::vector IDiskRemote::getRemotePaths(const String & local_path) const void IDiskRemote::getRemotePathsRecursive(const String & local_path, std::vector & paths_map) { + /// Protect against concurrent delition of files (for example because of a merge). if (metadata_disk->isFile(local_path)) { - /// File could be concurrently deleted (for example because of a merge). try { paths_map.emplace_back(local_path, getRemotePaths(local_path)); @@ -373,9 +373,19 @@ void IDiskRemote::getRemotePathsRecursive(const String & local_path, std::vector } else { - /// Metadata disk is currently only DislLocal. DiskLocal::iterateDirectory will return empty directory iterator - /// if file does not exist (even on cocnurrent file delition), - for (auto it = iterateDirectory(local_path); it->isValid(); it->next()) + DiskDirectoryIteratorPtr it; + try + { + it = iterateDirectory(local_path); + } + catch (const fs::filesystem_error & e) + { + if (e.code() == std::errc::no_such_file_or_directory) + return; + throw; + } + + for (; it->isValid(); it->next()) IDiskRemote::getRemotePathsRecursive(fs::path(local_path) / it->name(), paths_map); } }