Remove ReadOnlyMetadataStorage

Throw exceptions from IMetadataStorage instead to avoid introducing
extra abstractions.

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>

src/Disks/ObjectStorages/IMetadataStorage.h

@@ -11,10 +11,16 @@
 #include <Disks/DirectoryIterator.h>
 #include <Disks/WriteMode.h>
 #include <Disks/ObjectStorages/IObjectStorage.h>
+#include <Common/ErrorCodes.h>
 
 namespace DB
 {
 
+namespace ErrorCodes
+{
+    extern const int NOT_IMPLEMENTED;
+}
+
 class IMetadataStorage;
 
 /// Tries to provide some "transactions" interface, which allow
@@ -33,32 +39,71 @@ public:
     /// General purpose methods
 
     /// Write metadata string to file
-    virtual void writeStringToFile(const std::string & path, const std::string & data) = 0;
+    virtual void writeStringToFile(const std::string & /* path */, const std::string & /* data */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void setLastModified(const std::string & path, const Poco::Timestamp & timestamp) = 0;
+    virtual void setLastModified(const std::string & /* path */, const Poco::Timestamp & /* timestamp */)
+    {
+        throwNotImplemented();
+    }
 
     virtual bool supportsChmod() const = 0;
-    virtual void chmod(const String & path, mode_t mode) = 0;
+    virtual void chmod(const String & /* path */, mode_t /* mode */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void setReadOnly(const std::string & path) = 0;
+    virtual void setReadOnly(const std::string & /* path */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void unlinkFile(const std::string & path) = 0;
+    virtual void unlinkFile(const std::string & /* path */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void createDirectory(const std::string & path) = 0;
+    virtual void createDirectory(const std::string & /* path */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void createDirectoryRecursive(const std::string & path) = 0;
+    virtual void createDirectoryRecursive(const std::string & /* path */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void removeDirectory(const std::string & path) = 0;
+    virtual void removeDirectory(const std::string & /* path */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void removeRecursive(const std::string & path) = 0;
+    virtual void removeRecursive(const std::string & /* path */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void createHardLink(const std::string & path_from, const std::string & path_to) = 0;
+    virtual void createHardLink(const std::string & /* path_from */, const std::string & /* path_to */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void moveFile(const std::string & path_from, const std::string & path_to) = 0;
+    virtual void moveFile(const std::string & /* path_from */, const std::string & /* path_to */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void moveDirectory(const std::string & path_from, const std::string & path_to) = 0;
+    virtual void moveDirectory(const std::string & /* path_from */, const std::string & /* path_to */)
+    {
+        throwNotImplemented();
+    }
 
-    virtual void replaceFile(const std::string & path_from, const std::string & path_to) = 0;
+    virtual void replaceFile(const std::string & /* path_from */, const std::string & /* path_to */)
+    {
+        throwNotImplemented();
+    }
 
     /// Metadata related methods
 
@@ -69,7 +114,10 @@ public:
     virtual void createMetadataFile(const std::string & path, const std::string & blob_name, uint64_t size_in_bytes) = 0;
 
     /// Add to new blob to metadata file (way to implement appends)
-    virtual void addBlobToMetadata(const std::string & path, const std::string & blob_name, uint64_t size_in_bytes) = 0;
+    virtual void addBlobToMetadata(const std::string & /* path */, const std::string & /* blob_name */, uint64_t /* size_in_bytes */)
+    {
+        throwNotImplemented();
+    }
 
     /// Unlink metadata file and do something special if required
     /// By default just remove file (unlink file).
@@ -79,6 +127,12 @@ public:
     }
 
     virtual ~IMetadataTransaction() = default;
+
+private:
+    [[noreturn]] static void throwNotImplemented()
+    {
+        throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Operation is not implemented");
+    }
 };
 
 using MetadataTransactionPtr = std::shared_ptr<IMetadataTransaction>;
@@ -106,12 +160,18 @@ public:
 
     virtual Poco::Timestamp getLastModified(const std::string & path) const = 0;
 
-    virtual time_t getLastChanged(const std::string & path) const = 0;
+    virtual time_t getLastChanged(const std::string & /* path */) const
+    {
+        throwNotImplemented();
+    }
 
     virtual bool supportsChmod() const = 0;
 
     virtual bool supportsStat() const = 0;
-    virtual struct stat stat(const String & path) const = 0;
+    virtual struct stat stat(const String & /* path */) const
+    {
+        throwNotImplemented();
+    }
 
     virtual std::vector<std::string> listDirectory(const std::string & path) const = 0;
 
@@ -120,20 +180,32 @@ public:
     virtual uint32_t getHardlinkCount(const std::string & path) const = 0;
 
     /// Read metadata file to string from path
-    virtual std::string readFileToString(const std::string & path) const = 0;
+    virtual std::string readFileToString(const std::string & /* path */) const
+    {
+        throwNotImplemented();
+    }
 
     virtual ~IMetadataStorage() = default;
 
     /// ==== More specific methods. Previous were almost general purpose. ====
 
     /// Read multiple metadata files into strings and return mapping from file_path -> metadata
-    virtual std::unordered_map<std::string, std::string> getSerializedMetadata(const std::vector<String> & file_paths) const = 0;
+    virtual std::unordered_map<std::string, std::string> getSerializedMetadata(const std::vector<String> & /* file_paths */) const
+    {
+        throwNotImplemented();
+    }
 
     /// Return object information (absolute_path, bytes_size, ...) for metadata path.
     /// object_storage_path is absolute.
     virtual StoredObjects getStorageObjects(const std::string & path) const = 0;
 
     virtual std::string getObjectStorageRootPath() const = 0;
+
+private:
+    [[noreturn]] static void throwNotImplemented()
+    {
+        throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Operation is not implemented");
+    }
 };
 
 using MetadataStoragePtr = std::shared_ptr<IMetadataStorage>;

src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorage.h

@@ -2,7 +2,6 @@
 
 #include <Disks/IDisk.h>
 #include <Disks/ObjectStorages/IMetadataStorage.h>
-#include <Disks/ObjectStorages/ReadOnlyMetadataStorage.h>
 #include <Disks/ObjectStorages/MetadataFromDiskTransactionState.h>
 #include <Disks/ObjectStorages/MetadataStorageFromDiskTransactionOperations.h>
 
@@ -21,10 +20,7 @@ namespace DB
 /// It is used to allow BACKUP/RESTORE to ObjectStorage (S3/...) with the same
 /// structure as on disk MergeTree, and does not requires metadata from local
 /// disk to restore.
-///
+class MetadataStorageFromPlainObjectStorage final : public IMetadataStorage
-/// NOTE: Inheritance from ReadOnlyMetadataStorage is used here to throw
-/// NOT_IMPLEMENTED error for lost of unsupported methods (mtime/move/stat/...)
-class MetadataStorageFromPlainObjectStorage final : public ReadOnlyMetadataStorage
 {
 private:
     friend class MetadataStorageFromPlainObjectStorageTransaction;
@@ -59,11 +55,25 @@ public:
 
     std::string getObjectStorageRootPath() const override { return object_storage_root_path; }
 
+    Poco::Timestamp getLastModified(const std::string & /* path */) const override
+    {
+        /// Required by MergeTree
+        return {};
+    }
+
+    uint32_t getHardlinkCount(const std::string & /* path */) const override
+    {
+        return 1;
+    }
+
+    bool supportsChmod() const override { return false; }
+    bool supportsStat() const override { return false; }
+
 private:
     std::filesystem::path getAbsolutePath(const std::string & path) const;
 };
 
-class MetadataStorageFromPlainObjectStorageTransaction final : public ReadOnlyMetadataTransaction
+class MetadataStorageFromPlainObjectStorageTransaction final : public IMetadataTransaction
 {
 private:
     const MetadataStorageFromPlainObjectStorage & metadata_storage;
@@ -74,9 +84,7 @@ public:
         : metadata_storage(metadata_storage_)
     {}
 
-    ~MetadataStorageFromPlainObjectStorageTransaction() override = default;
+    const IMetadataStorage & getStorageForNonTransactionalReads() const override;
-
-    const IMetadataStorage & getStorageForNonTransactionalReads() const final;
 
     void addBlobToMetadata(const std::string & path, const std::string & blob_name, uint64_t size_in_bytes) override;
 
@@ -97,6 +105,13 @@ public:
     void unlinkFile(const std::string & path) override;
 
     void unlinkMetadata(const std::string & path) override;
+
+    void commit() override
+    {
+        /// Nothing to commit.
+    }
+
+    bool supportsChmod() const override { return false; }
 };
 
 }

src/Disks/ObjectStorages/ReadOnlyMetadataStorage.h

@@ -1,146 +0,0 @@
-#pragma once
-
-#include <Disks/ObjectStorages/IMetadataStorage.h>
-#include <Common/ErrorCodes.h>
-
-namespace DB
-{
-
-namespace ErrorCodes
-{
-    extern const int NOT_IMPLEMENTED;
-}
-
-class ReadOnlyMetadataStorage;
-
-/// Transaction for read-only storage, throws NOT_IMPLEMENTED error.
-/// Can be used to add limited read-only support of MergeTree.
-class ReadOnlyMetadataTransaction : public IMetadataTransaction
-{
-public:
-    void commit() override
-    {
-        /// Noop, nothing to commit.
-    }
-
-    void createEmptyMetadataFile(const std::string & /* path */) override
-    {
-        throwNotAllowed();
-    }
-    void createMetadataFile(const std::string & /* path */, const std::string & /* blob_name */, uint64_t /* size_in_bytes */) override
-    {
-        throwNotAllowed();
-    }
-    void writeStringToFile(const std::string & /* path */, const std::string & /* data */) override
-    {
-        throwNotAllowed();
-    }
-    void setLastModified(const std::string & /* path */, const Poco::Timestamp & /* timestamp */) override
-    {
-        throwNotAllowed();
-    }
-    void chmod(const String & /* path */, mode_t /* mode */) override
-    {
-        throwNotAllowed();
-    }
-    void setReadOnly(const std::string & /* path */) override
-    {
-        throwNotAllowed();
-    }
-    void unlinkFile(const std::string & /* path */) override
-    {
-        throwNotAllowed();
-    }
-    void removeDirectory(const std::string & /* path */) override
-    {
-        throwNotAllowed();
-    }
-    void removeRecursive(const std::string & /* path */) override
-    {
-        throwNotAllowed();
-    }
-    void createHardLink(const std::string & /* path_from */, const std::string & /* path_to */) override
-    {
-        throwNotAllowed();
-    }
-    void moveFile(const std::string & /* path_from */, const std::string & /* path_to */) override
-    {
-        throwNotAllowed();
-    }
-    void moveDirectory(const std::string & /* path_from */, const std::string & /* path_to */) override
-    {
-        throwNotAllowed();
-    }
-    void replaceFile(const std::string & /* path_from */, const std::string & /* path_to */) override
-    {
-        throwNotAllowed();
-    }
-    void createDirectory(const std::string & /* path */) override
-    {
-        throwNotAllowed();
-    }
-    void createDirectoryRecursive(const std::string & /* path */) override
-    {
-        throwNotAllowed();
-    }
-    void addBlobToMetadata(const std::string & /* path */, const std::string & /* blob_name */, uint64_t /* size_in_bytes */) override
-    {
-        throwNotAllowed();
-    }
-    void unlinkMetadata(const std::string & /* path */) override
-    {
-        throwNotAllowed();
-    }
-
-    bool supportsChmod() const override { return false; }
-
-private:
-    [[noreturn]] static void throwNotAllowed()
-    {
-        throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Transaction for read-only storage is not supported");
-    }
-};
-
-/// Readonly storage, throws NOT_IMPLEMENTED error.
-/// Can be used to add limited read-only support of MergeTree.
-class ReadOnlyMetadataStorage : public IMetadataStorage
-{
-public:
-    Poco::Timestamp getLastModified(const std::string & /* path */) const override
-    {
-        /// Required by MergeTree
-        return {};
-    }
-    uint32_t getHardlinkCount(const std::string & /* path */) const override
-    {
-        return 1;
-    }
-
-    struct stat stat(const String & /* path */) const override
-    {
-        throwNotImplemented();
-    }
-    time_t getLastChanged(const std::string & /* path */) const override
-    {
-        throwNotImplemented();
-    }
-    std::string readFileToString(const std::string & /* path */) const override
-    {
-        throwNotImplemented();
-    }
-    std::unordered_map<std::string, std::string> getSerializedMetadata(const std::vector<String> & /* file_paths */) const override
-    {
-        throwNotImplemented();
-    }
-
-    bool supportsChmod() const override { return false; }
-    bool supportsStat() const override { return false; }
-
-private:
-    [[noreturn]] static void throwNotImplemented()
-    {
-        throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Operation is ont implemented");
-    }
-};
-
-}

src/Disks/ObjectStorages/Web/MetadataStorageFromStaticFilesWebServer.h

@@ -1,7 +1,6 @@
 #pragma once
 
 #include <Disks/ObjectStorages/IMetadataStorage.h>
-#include <Disks/ObjectStorages/ReadOnlyMetadataStorage.h>
 #include <Disks/ObjectStorages/MetadataFromDiskTransactionState.h>
 #include <Disks/ObjectStorages/Web/WebObjectStorage.h>
 #include <Disks/IDisk.h>
@@ -10,7 +9,7 @@
 namespace DB
 {
 
-class MetadataStorageFromStaticFilesWebServer final : public ReadOnlyMetadataStorage
+class MetadataStorageFromStaticFilesWebServer final : public IMetadataStorage
 {
 private:
     friend class MetadataStorageFromStaticFilesWebServerTransaction;
@@ -46,9 +45,22 @@ public:
     std::string getObjectStorageRootPath() const override { return ""; }
 
     struct stat stat(const String & /* path */) const override { return {}; }
+
+    Poco::Timestamp getLastModified(const std::string & /* path */) const override
+    {
+        /// Required by MergeTree
+        return {};
+    }
+    uint32_t getHardlinkCount(const std::string & /* path */) const override
+    {
+        return 1;
+    }
+
+    bool supportsChmod() const override { return false; }
+    bool supportsStat() const override { return false; }
 };
 
-class MetadataStorageFromStaticFilesWebServerTransaction final : public ReadOnlyMetadataTransaction
+class MetadataStorageFromStaticFilesWebServerTransaction final : public IMetadataTransaction
 {
 private:
     DiskPtr disk;
@@ -60,8 +72,6 @@ public:
         : metadata_storage(metadata_storage_)
     {}
 
-    ~MetadataStorageFromStaticFilesWebServerTransaction() override = default;
-
     const IMetadataStorage & getStorageForNonTransactionalReads() const override;
 
     void createEmptyMetadataFile(const std::string & /* path */) override
@@ -77,6 +87,13 @@ public:
     void createDirectory(const std::string & path) override;
 
     void createDirectoryRecursive(const std::string & path) override;
+
+    void commit() override
+    {
+        /// Nothing to commit.
+    }
+
+    bool supportsChmod() const override { return false; }
 };
 
 }