Fixed implementation of DiskLocal::moveTo

.clang-format

@@ -87,7 +87,7 @@ PenaltyBreakComment: 300
 PenaltyBreakFirstLessLess: 120
 PenaltyBreakString: 1000
 PenaltyExcessCharacter: 1000000
-PenaltyReturnTypeOnItsOwnLine: 1000
+PenaltyReturnTypeOnItsOwnLine: 60
 SpaceAfterCStyleCast: false
 SpaceBeforeAssignmentOperators: true
 SpaceBeforeParens: ControlStatements

dbms/src/Disks/DiskFactory.cpp

@@ -14,13 +14,13 @@ DiskFactory & DiskFactory::instance()
     return factory;
 }
 
-void DiskFactory::registerDisk(const String & disk_type, DB::DiskFactory::Creator creator)
+void DiskFactory::registerDiskType(const String & disk_type, DB::DiskFactory::Creator creator)
 {
     if (!registry.emplace(disk_type, creator).second)
         throw Exception("DiskFactory: the disk type '" + disk_type + "' is not unique", ErrorCodes::LOGICAL_ERROR);
 }
 
-DiskPtr DiskFactory::get(
+DiskPtr DiskFactory::create(
     const String & name,
     const Poco::Util::AbstractConfiguration & config,
     const String & config_prefix,

dbms/src/Disks/DiskFactory.h

@@ -13,20 +13,25 @@ namespace DB
 {
 class Context;
 
+/**
+ * Disk factory. Responsible for creating new disk objects.
+ */
 class DiskFactory final : private boost::noncopyable
 {
 public:
     using Creator = std::function<DiskPtr(
-        const String & name, const Poco::Util::AbstractConfiguration & config,
+        const String & name,
+        const Poco::Util::AbstractConfiguration & config,
         const String & config_prefix,
         const Context & context)>;
 
     static DiskFactory & instance();
 
-    void registerDisk(const String & disk_type, Creator creator);
+    void registerDiskType(const String & disk_type, Creator creator);
 
-    DiskPtr get(
-        const String & name, const Poco::Util::AbstractConfiguration & config,
+    DiskPtr create(
+        const String & name,
+        const Poco::Util::AbstractConfiguration & config,
         const String & config_prefix,
         const Context & context) const;
 

dbms/src/Disks/DiskLocal.cpp

 #include "DiskLocal.h"
 #include "DiskFactory.h"
 
+#include <Interpreters/Context.h>
 #include <Common/filesystemHelpers.h>
 #include <Common/quoteString.h>
-#include <Interpreters/Context.h>
 
 
 namespace DB
@@ -76,6 +76,52 @@ DiskFilePtr DiskLocal::file(const String & path_) const
 }
 
 
+DiskLocalFile::DiskLocalFile(const DiskPtr & disk_ptr_, const String & rel_path_)
+    : IDiskFile(disk_ptr_, rel_path_), file(disk_ptr->getPath() + rel_path)
+{
+}
+
+bool DiskLocalFile::exists() const
+{
+    return file.exists();
+}
+
+bool DiskLocalFile::isDirectory() const
+{
+    return file.isDirectory();
+}
+
+void DiskLocalFile::createDirectory()
+{
+    file.createDirectory();
+}
+
+void DiskLocalFile::createDirectories()
+{
+    file.createDirectories();
+}
+
+void DiskLocalFile::moveTo(const String & new_path)
+{
+    file.renameTo(disk_ptr->getPath() + new_path);
+}
+
+void DiskLocalFile::copyTo(const String & new_path)
+{
+    file.copyTo(disk_ptr->getPath() + new_path);
+}
+
+std::unique_ptr<ReadBuffer> DiskLocalFile::read() const
+{
+    return std::make_unique<ReadBufferFromFile>(file.path());
+}
+
+std::unique_ptr<WriteBuffer> DiskLocalFile::write()
+{
+    return std::make_unique<WriteBufferFromFile>(file.path());
+}
+
+
 DiskDirectoryIteratorImplPtr DiskLocalFile::iterateDirectory()
 {
     return std::make_unique<DiskLocalDirectoryIterator>(shared_from_this());
@@ -188,7 +234,7 @@ void registerDiskLocal(DiskFactory & factory)
 
         return std::make_shared<const DiskLocal>(name, path, keep_free_space_bytes);
     };
-    factory.registerDisk("local", creator);
+    factory.registerDiskType("local", creator);
 }
 
 }

dbms/src/Disks/DiskLocal.h

@@ -65,26 +65,23 @@ using DiskLocalPtr = std::shared_ptr<const DiskLocal>;
 class DiskLocalFile : public IDiskFile
 {
 public:
-    DiskLocalFile(const DiskPtr & disk_ptr_, const String & rel_path_)
-        : IDiskFile(disk_ptr_, rel_path_), file(disk_ptr->getPath() + rel_path)
-    {
-    }
+    DiskLocalFile(const DiskPtr & disk_ptr_, const String & rel_path_);
 
-    bool exists() const override { return file.exists(); }
+    bool exists() const override;
 
-    bool isDirectory() const override { return file.isDirectory(); }
+    bool isDirectory() const override;
 
-    void createDirectory() override { file.createDirectory(); }
+    void createDirectory() override;
 
-    void createDirectories() override { file.createDirectories(); }
+    void createDirectories() override;
 
-    void moveTo(const String & new_path) override { file.renameTo(new_path); }
+    void moveTo(const String & new_path) override;
 
-    void copyTo(const String & new_path) override { file.copyTo(new_path); }
+    void copyTo(const String & new_path) override;
 
-    std::unique_ptr<ReadBuffer> read() const override { return std::make_unique<ReadBufferFromFile>(file.path()); }
+    std::unique_ptr<ReadBuffer> read() const override;
 
-    std::unique_ptr<WriteBuffer> write() override { return std::make_unique<WriteBufferFromFile>(file.path()); }
+    std::unique_ptr<WriteBuffer> write() override;
 
 private:
     DiskDirectoryIteratorImplPtr iterateDirectory() override;

dbms/src/Disks/DiskSpaceMonitor.cpp

 #include "DiskSpaceMonitor.h"
 #include "DiskFactory.h"
 #include "DiskLocal.h"
+
+#include <Interpreters/Context.h>
 #include <Common/escapeForFileName.h>
 #include <Common/quoteString.h>
-#include <Interpreters/Context.h>
 
 #include <set>
 #include <Poco/File.h>
@@ -11,7 +12,6 @@
 
 namespace DB
 {
-
 DiskSelector::DiskSelector(const Poco::Util::AbstractConfiguration & config, const String & config_prefix, const Context & context)
 {
     Poco::Util::AbstractConfiguration::Keys keys;
@@ -24,15 +24,14 @@ DiskSelector::DiskSelector(const Poco::Util::AbstractConfiguration & config, con
     for (const auto & disk_name : keys)
     {
         if (!std::all_of(disk_name.begin(), disk_name.end(), isWordCharASCII))
-            throw Exception("Disk name can contain only alphanumeric and '_' (" + disk_name + ")",
-                ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
+            throw Exception("Disk name can contain only alphanumeric and '_' (" + disk_name + ")", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
 
         if (disk_name == default_disk_name)
             has_default_disk = true;
 
         auto disk_config_prefix = config_prefix + "." + disk_name;
 
-        disks.emplace(disk_name, factory.get(disk_name, config, disk_config_prefix, context));
+        disks.emplace(disk_name, factory.create(disk_name, config, disk_config_prefix, context));
     }
     if (!has_default_disk)
         disks.emplace(default_disk_name, std::make_shared<const DiskLocal>(default_disk_name, context.getPath(), 0));
@@ -75,8 +74,9 @@ Volume::Volume(
     auto has_max_bytes = config.has(config_prefix + ".max_data_part_size_bytes");
     auto has_max_ratio = config.has(config_prefix + ".max_data_part_size_ratio");
     if (has_max_bytes && has_max_ratio)
-        throw Exception("Only one of 'max_data_part_size_bytes' and 'max_data_part_size_ratio' should be specified.",
-                        ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
+        throw Exception(
+            "Only one of 'max_data_part_size_bytes' and 'max_data_part_size_ratio' should be specified.",
+            ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
 
     if (has_max_bytes)
     {
@@ -86,8 +86,7 @@ Volume::Volume(
     {
         auto ratio = config.getDouble(config_prefix + ".max_data_part_size_ratio");
         if (ratio < 0)
-            throw Exception("'max_data_part_size_ratio' have to be not less then 0.",
-                            ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
+            throw Exception("'max_data_part_size_ratio' have to be not less then 0.", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
         UInt64 sum_size = 0;
         std::vector<UInt64> sizes;
         for (const auto & disk : disks)
@@ -98,16 +97,18 @@ Volume::Volume(
         max_data_part_size = static_cast<decltype(max_data_part_size)>(sum_size * ratio / disks.size());
         for (size_t i = 0; i < disks.size(); ++i)
             if (sizes[i] < max_data_part_size)
-                LOG_WARNING(logger, "Disk " << backQuote(disks[i]->getName()) << " on volume " << backQuote(config_prefix) <<
-                                    " have not enough space (" << formatReadableSizeWithBinarySuffix(sizes[i]) <<
-                                    ") for containing part the size of max_data_part_size (" <<
-                                    formatReadableSizeWithBinarySuffix(max_data_part_size) << ")");
+                LOG_WARNING(
+                    logger,
+                    "Disk " << backQuote(disks[i]->getName()) << " on volume " << backQuote(config_prefix) << " have not enough space ("
+                            << formatReadableSizeWithBinarySuffix(sizes[i]) << ") for containing part the size of max_data_part_size ("
+                            << formatReadableSizeWithBinarySuffix(max_data_part_size) << ")");
     }
     constexpr UInt64 MIN_PART_SIZE = 8u * 1024u * 1024u;
     if (max_data_part_size != 0 && max_data_part_size < MIN_PART_SIZE)
-        LOG_WARNING(logger, "Volume " << backQuote(name) << " max_data_part_size is too low ("
-            << formatReadableSizeWithBinarySuffix(max_data_part_size) << " < "
-            << formatReadableSizeWithBinarySuffix(MIN_PART_SIZE) << ")");
+        LOG_WARNING(
+            logger,
+            "Volume " << backQuote(name) << " max_data_part_size is too low (" << formatReadableSizeWithBinarySuffix(max_data_part_size)
+                      << " < " << formatReadableSizeWithBinarySuffix(MIN_PART_SIZE) << ")");
 }
 
 
@@ -158,7 +159,8 @@ StoragePolicy::StoragePolicy(
     for (const auto & attr_name : keys)
     {
         if (!std::all_of(attr_name.begin(), attr_name.end(), isWordCharASCII))
-            throw Exception("Volume name can contain only alphanumeric and '_' (" + attr_name + ")", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
+            throw Exception(
+                "Volume name can contain only alphanumeric and '_' (" + attr_name + ")", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
         volumes.push_back(std::make_shared<Volume>(attr_name, config, volumes_prefix + "." + attr_name, disks));
         if (volumes_names.find(attr_name) != volumes_names.end())
             throw Exception("Volumes names must be unique (" + attr_name + " duplicated)", ErrorCodes::UNKNOWN_POLICY);
@@ -175,7 +177,8 @@ StoragePolicy::StoragePolicy(
         for (const auto & disk : volume->disks)
         {
             if (disk_names.find(disk->getName()) != disk_names.end())
-                throw Exception("Duplicate disk '" + disk->getName() + "' in storage policy '" + name + "'", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
+                throw Exception(
+                    "Duplicate disk '" + disk->getName() + "' in storage policy '" + name + "'", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
 
             disk_names.insert(disk->getName());
         }
@@ -183,23 +186,18 @@ StoragePolicy::StoragePolicy(
 
     move_factor = config.getDouble(config_prefix + ".move_factor", 0.1);
     if (move_factor > 1)
-        throw Exception("Disk move factor have to be in [0., 1.] interval, but set to " + toString(move_factor),
-            ErrorCodes::LOGICAL_ERROR);
-
+        throw Exception("Disk move factor have to be in [0., 1.] interval, but set to " + toString(move_factor), ErrorCodes::LOGICAL_ERROR);
 }
 
 
 StoragePolicy::StoragePolicy(String name_, Volumes volumes_, double move_factor_)
-    : volumes(std::move(volumes_))
-    , name(std::move(name_))
-    , move_factor(move_factor_)
+    : volumes(std::move(volumes_)), name(std::move(name_)), move_factor(move_factor_)
 {
     if (volumes.empty())
         throw Exception("StoragePolicy must contain at least one Volume.", ErrorCodes::UNKNOWN_POLICY);
 
     if (move_factor > 1)
-        throw Exception("Disk move factor have to be in [0., 1.] interval, but set to " + toString(move_factor),
-            ErrorCodes::LOGICAL_ERROR);
+        throw Exception("Disk move factor have to be in [0., 1.] interval, but set to " + toString(move_factor), ErrorCodes::LOGICAL_ERROR);
 
     for (size_t i = 0; i < volumes.size(); ++i)
     {
@@ -316,8 +314,8 @@ StoragePolicySelector::StoragePolicySelector(
     for (const auto & name : keys)
     {
         if (!std::all_of(name.begin(), name.end(), isWordCharASCII))
-            throw Exception("StoragePolicy name can contain only alphanumeric and '_' (" + name + ")",
-                ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
+            throw Exception(
+                "StoragePolicy name can contain only alphanumeric and '_' (" + name + ")", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG);
 
         policies.emplace(name, std::make_shared<StoragePolicy>(name, config, config_prefix + "." + name, disks));
         LOG_INFO(&Logger::get("StoragePolicySelector"), "Storage policy " << backQuote(name) << " loaded");
@@ -330,10 +328,7 @@ StoragePolicySelector::StoragePolicySelector(
     /// Add default policy if it's not specified explicetly
     if (policies.find(default_storage_policy_name) == policies.end())
     {
-        auto default_volume = std::make_shared<Volume>(
-            default_volume_name,
-            std::vector<DiskPtr>{disks[default_disk_name]},
-            0);
+        auto default_volume = std::make_shared<Volume>(default_volume_name, std::vector<DiskPtr>{disks[default_disk_name]}, 0);
 
         auto default_policy = std::make_shared<StoragePolicy>(default_storage_policy_name, Volumes{default_volume}, 0.0);
         policies.emplace(default_storage_policy_name, default_policy);

dbms/src/Disks/DiskSpaceMonitor.h

 #pragma once
 
-#include <common/logger_useful.h>
+#include <Disks/IDisk.h>
+#include <IO/WriteHelpers.h>
 #include <Common/CurrentMetrics.h>
 #include <Common/Exception.h>
 #include <Common/formatReadable.h>
-#include <Disks/IDisk.h>
-#include <IO/WriteHelpers.h>
+#include <common/logger_useful.h>
 
 #include <memory>
 #include <mutex>
@@ -38,7 +38,7 @@ public:
     /// Get disk by name
     const DiskPtr & operator[](const String & name) const;
 
-    /// Get all disks name
+    /// Get all disks with names
     const auto & getDisksMap() const { return disks; }
 
 private:
@@ -62,7 +62,10 @@ public:
     }
 
     Volume(
-        String name_, const Poco::Util::AbstractConfiguration & config, const String & config_prefix, const DiskSelector & disk_selector);
+        String name_,
+        const Poco::Util::AbstractConfiguration & config,
+        const String & config_prefix,
+        const DiskSelector & disk_selector);
 
     /// Uses Round-robin to choose disk for reservation.
     /// Returns valid reservation or nullptr if there is no space left on any disk.

dbms/src/Disks/IDisk.h

 #pragma once
 
+#include <Core/Types.h>
 #include <Common/CurrentMetrics.h>
 #include <Common/Exception.h>
-#include <Core/Types.h>
 
 #include <memory>
 #include <utility>
@@ -44,6 +44,13 @@ public:
 
 using SpacePtr = std::shared_ptr<const Space>;
 
+/**
+ * A unit of storage persisting data and metadata.
+ * Abstract underlying storage technology.
+ * Responsible for:
+ * - file management;
+ * - space accounting and reservation.
+ */
 class IDisk : public Space
 {
 public:

dbms/src/Disks/registerDisks.cpp

@@ -2,7 +2,6 @@
 
 namespace DB
 {
-
 void registerDiskLocal(DiskFactory & factory);
 
 void registerDisks()

dbms/src/Disks/registerDisks.h

@@ -2,7 +2,6 @@
 
 namespace DB
 {
-
 void registerDisks();
 
 }