From 096d117f6801385fe4dc38f42e343206916dae28 Mon Sep 17 00:00:00 2001 From: kssenii Date: Sat, 22 May 2021 00:12:46 +0300 Subject: [PATCH] Review fixes --- src/Common/Config/ConfigProcessor.cpp | 11 +++++------ src/Common/ErrorCodes.cpp | 1 - src/Common/createFile.cpp | 4 ++-- src/Databases/DatabaseOnDisk.cpp | 2 -- src/Interpreters/DatabaseCatalog.cpp | 2 +- src/Interpreters/loadMetadata.cpp | 6 +++--- src/Storages/StorageReplicatedMergeTree.cpp | 2 +- 7 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/Common/Config/ConfigProcessor.cpp b/src/Common/Config/ConfigProcessor.cpp index 396334c6a8b..598f64e0d99 100644 --- a/src/Common/Config/ConfigProcessor.cpp +++ b/src/Common/Config/ConfigProcessor.cpp @@ -416,16 +416,15 @@ ConfigProcessor::Files ConfigProcessor::getConfigMergeFiles(const std::string & std::set merge_dirs; /// Add path_to_config/config_name.d dir - merge_dir_path = merge_dir_path.parent_path() / (merge_dir_path.stem().string() + ".d"); + merge_dir_path.replace_extension("d"); merge_dirs.insert(merge_dir_path); /// Add path_to_config/conf.d dir - merge_dir_path = merge_dir_path.parent_path() / "conf.d"; + merge_dir_path.replace_filename("conf.d"); merge_dirs.insert(merge_dir_path); for (const std::string & merge_dir_name : merge_dirs) { - fs::path merge_dir(merge_dir_name); - if (!fs::exists(merge_dir) || !is_directory(merge_dir)) + if (!fs::exists(merge_dir_name) || !fs::is_directory(merge_dir_name)) continue; for (fs::directory_iterator it(merge_dir_name); it != fs::directory_iterator(); ++it) @@ -635,7 +634,7 @@ void ConfigProcessor::savePreprocessedConfig(const LoadedConfig & loaded_config, fs::path parent_path = fs::path(loaded_config.config_path).parent_path(); preprocessed_dir = parent_path.string(); fs::path fs_new_path(new_path); - fs_new_path = fs_new_path.parent_path() / (fs_new_path.stem().string() + PREPROCESSED_SUFFIX + fs_new_path.extension().string()); + fs_new_path.replace_filename(fs_new_path.stem().string() + PREPROCESSED_SUFFIX + fs_new_path.extension().string()); new_path = fs_new_path.string(); } else @@ -652,7 +651,7 @@ void ConfigProcessor::savePreprocessedConfig(const LoadedConfig & loaded_config, preprocessed_path = (fs::path(preprocessed_dir) / fs::path(new_path)).string(); auto preprocessed_path_parent = fs::path(preprocessed_path).parent_path(); - if (!preprocessed_path_parent.string().empty()) + if (!preprocessed_path_parent.empty()) fs::create_directories(preprocessed_path_parent); } DOMWriter().writeNode(preprocessed_path, loaded_config.preprocessed_xml); diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index d3cd812ef64..dccc20d325b 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -553,7 +553,6 @@ M(583, ILLEGAL_PROJECTION) \ M(584, PROJECTION_NOT_USED) \ \ - M(996, OPERATION_NOT_PERMITTED) \ M(997, CANNOT_CREATE_FILE) \ M(998, POSTGRESQL_CONNECTION_FAILURE) \ M(999, KEEPER_EXCEPTION) \ diff --git a/src/Common/createFile.cpp b/src/Common/createFile.cpp index 83ab9c36f4b..74f8f9cf980 100644 --- a/src/Common/createFile.cpp +++ b/src/Common/createFile.cpp @@ -11,7 +11,7 @@ namespace DB namespace ErrorCodes { extern const int FILE_ALREADY_EXISTS; -extern const int OPERATION_NOT_PERMITTED; +extern const int PATH_ACCESS_DENIED; extern const int NOT_ENOUGH_SPACE; extern const int CANNOT_CREATE_FILE; } @@ -26,7 +26,7 @@ namespace FS case EEXIST: throw DB::Exception(DB::ErrorCodes::FILE_ALREADY_EXISTS, "File {} already exist", path); case EPERM: - throw DB::Exception(DB::ErrorCodes::OPERATION_NOT_PERMITTED, "Not enough permissions to create file {}", path); + throw DB::Exception(DB::ErrorCodes::PATH_ACCESS_DENIED, "Not enough permissions to create file {}", path); case ENOSPC: throw DB::Exception(DB::ErrorCodes::NOT_ENOUGH_SPACE, "Not enough space to create file {}", path); case ENAMETOOLONG: diff --git a/src/Databases/DatabaseOnDisk.cpp b/src/Databases/DatabaseOnDisk.cpp index 8393f9e81c0..f50adf54c3f 100644 --- a/src/Databases/DatabaseOnDisk.cpp +++ b/src/Databases/DatabaseOnDisk.cpp @@ -19,8 +19,6 @@ #include #include #include -#include -#include #include namespace fs = std::filesystem; diff --git a/src/Interpreters/DatabaseCatalog.cpp b/src/Interpreters/DatabaseCatalog.cpp index ca473b4aac6..0794cb52a49 100644 --- a/src/Interpreters/DatabaseCatalog.cpp +++ b/src/Interpreters/DatabaseCatalog.cpp @@ -356,7 +356,7 @@ DatabasePtr DatabaseCatalog::detachDatabase(const String & database_name, bool d /// Old ClickHouse versions did not store database.sql files fs::path database_metadata_file = fs::path(getContext()->getPath()) / "metadata" / (escapeForFileName(database_name) + ".sql"); if (fs::exists(database_metadata_file)) - fs::remove_all(database_metadata_file); + fs::remove(database_metadata_file); } return db; diff --git a/src/Interpreters/loadMetadata.cpp b/src/Interpreters/loadMetadata.cpp index c109c53af20..0a84cdca309 100644 --- a/src/Interpreters/loadMetadata.cpp +++ b/src/Interpreters/loadMetadata.cpp @@ -112,15 +112,15 @@ void loadMetadata(ContextPtr context, const String & default_database_name) if (!it->is_directory()) { /// TODO: DETACH DATABASE PERMANENTLY ? - if (endsWith(current_file, ".sql")) + if (fs::path(current_file).extension() == ".sql") { String db_name = current_file.substr(0, current_file.size() - 4); if (db_name != DatabaseCatalog::SYSTEM_DATABASE) - databases.emplace(unescapeForFileName(db_name), path + "/" + db_name); + databases.emplace(unescapeForFileName(db_name), fs::path(path) / db_name); } /// Temporary fails may be left from previous server runs. - if (endsWith(current_file, ".tmp")) + if (fs::path(current_file).extension() == ".tmp") { LOG_WARNING(log, "Removing temporary file {}", it->path().string()); try diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index 2a34ae3fd07..88c7edc948b 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -6532,7 +6532,7 @@ void StorageReplicatedMergeTree::movePartitionToTable(const StoragePtr & dest_ta ops.emplace_back(zkutil::makeCheckRequest(alter_partition_version_path, alter_partition_version_stat.version)); ops.emplace_back(zkutil::makeSetRequest(alter_partition_version_path, "", -1)); /// Just update version, because merges assignment relies on it - ops.emplace_back(zkutil::makeSetRequest(fs:path(dest_table_storage->zookeeper_path) / "log", "", -1)); + ops.emplace_back(zkutil::makeSetRequest(fs::path(dest_table_storage->zookeeper_path) / "log", "", -1)); ops.emplace_back(zkutil::makeCreateRequest(fs::path(dest_table_storage->zookeeper_path) / "log/log-", entry.toString(), zkutil::CreateMode::PersistentSequential));