From 7be9744babc52f37be9d59c589dad99d155e07be Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Fri, 30 Sep 2022 19:29:03 +0200 Subject: [PATCH] Remove confusing warning when inserting with perform_ttl_move_on_insert=false. --- src/Disks/IDisk.h | 7 +++ src/Disks/IStoragePolicy.h | 1 + src/Disks/IVolume.h | 3 ++ src/Disks/StoragePolicy.cpp | 9 ++++ src/Disks/StoragePolicy.h | 3 ++ src/Storages/MergeTree/MergeTreeData.cpp | 65 ++++++++++++------------ src/Storages/MergeTree/MergeTreeData.h | 9 ++-- 7 files changed, 62 insertions(+), 35 deletions(-) diff --git a/src/Disks/IDisk.h b/src/Disks/IDisk.h index de7b9181533..ba843235345 100644 --- a/src/Disks/IDisk.h +++ b/src/Disks/IDisk.h @@ -74,6 +74,10 @@ public: /// Returns valid reservation or nullptr when failure. virtual ReservationPtr reserve(UInt64 bytes) = 0; + /// Whether this is a disk or a volume. + virtual bool isDisk() const { return false; } + virtual bool isVolume() const { return false; } + virtual ~Space() = default; }; @@ -108,6 +112,9 @@ public: { } + /// This is a disk. + bool isDisk() const override { return true; } + virtual DiskTransactionPtr createTransaction(); /// Root path for all files stored on the disk. diff --git a/src/Disks/IStoragePolicy.h b/src/Disks/IStoragePolicy.h index a5d1120f377..8d14a26691b 100644 --- a/src/Disks/IStoragePolicy.h +++ b/src/Disks/IStoragePolicy.h @@ -55,6 +55,7 @@ public: /// Get volume by index. virtual VolumePtr getVolume(size_t index) const = 0; virtual VolumePtr tryGetVolumeByName(const String & volume_name) const = 0; + virtual VolumePtr tryGetVolumeByDisk(const DiskPtr & disk_ptr) const = 0; VolumePtr getVolumeByName(const String & volume_name) const; /// Checks if storage policy can be replaced by another one. virtual void checkCompatibleWith(const StoragePolicyPtr & new_storage_policy) const = 0; diff --git a/src/Disks/IVolume.h b/src/Disks/IVolume.h index 26d4c96f481..ada28caa960 100644 --- a/src/Disks/IVolume.h +++ b/src/Disks/IVolume.h @@ -66,6 +66,9 @@ public: virtual ReservationPtr reserve(UInt64 bytes) override = 0; + /// This is a volume. + bool isVolume() const override { return true; } + /// Volume name from config const String & getName() const override { return name; } virtual VolumeType getType() const = 0; diff --git a/src/Disks/StoragePolicy.cpp b/src/Disks/StoragePolicy.cpp index 1c8c522a568..3662a9732b3 100644 --- a/src/Disks/StoragePolicy.cpp +++ b/src/Disks/StoragePolicy.cpp @@ -322,6 +322,15 @@ size_t StoragePolicy::getVolumeIndexByDisk(const DiskPtr & disk_ptr) const } +VolumePtr StoragePolicy::tryGetVolumeByDisk(const DiskPtr & disk_ptr) const +{ + auto it = volume_index_by_disk_name.find(disk_ptr->getName()); + if (it == volume_index_by_disk_name.end()) + return nullptr; + return getVolume(it->second); +} + + void StoragePolicy::buildVolumeIndices() { for (size_t index = 0; index < volumes.size(); ++index) diff --git a/src/Disks/StoragePolicy.h b/src/Disks/StoragePolicy.h index b0e92bdad67..fd0169a6ebe 100644 --- a/src/Disks/StoragePolicy.h +++ b/src/Disks/StoragePolicy.h @@ -85,6 +85,9 @@ public: VolumePtr tryGetVolumeByName(const String & volume_name) const override; + /// Finds a volume which contains a specified disk. + VolumePtr tryGetVolumeByDisk(const DiskPtr & disk_ptr) const override; + /// Checks if storage policy can be replaced by another one. void checkCompatibleWith(const StoragePolicyPtr & new_storage_policy) const override; diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index bd7ccf3f53e..041b5b46ad9 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -4917,24 +4917,33 @@ ReservationPtr MergeTreeData::tryReserveSpacePreferringTTLRules( if (move_ttl_entry) { LOG_TRACE(log, "Got move TTL entry, will try to reserver destination for move"); - SpacePtr destination_ptr = getDestinationForMoveTTL(*move_ttl_entry, is_insert); + SpacePtr destination_ptr = getDestinationForMoveTTL(*move_ttl_entry); + bool perform_ttl_move_on_insert = is_insert && destination_ptr && shouldPerformTTLMoveOnInsert(destination_ptr); + if (!destination_ptr) { if (move_ttl_entry->destination_type == DataDestinationType::VOLUME && !move_ttl_entry->if_exists) LOG_WARNING( log, - "Would like to reserve space on volume '{}' by TTL rule of table '{}' but volume was not found or rule is not " - "applicable at the moment", + "Would like to reserve space on volume '{}' by TTL rule of table '{}' but volume was not found", move_ttl_entry->destination_name, *std::atomic_load(&log_name)); else if (move_ttl_entry->destination_type == DataDestinationType::DISK && !move_ttl_entry->if_exists) LOG_WARNING( log, - "Would like to reserve space on disk '{}' by TTL rule of table '{}' but disk was not found or rule is not applicable " - "at the moment", + "Would like to reserve space on disk '{}' by TTL rule of table '{}' but disk was not found", move_ttl_entry->destination_name, *std::atomic_load(&log_name)); } + else if (is_insert && !perform_ttl_move_on_insert) + { + LOG_TRACE( + log, + "TTL move on insert to {} {} for table {} is disabled", + (move_ttl_entry->destination_type == DataDestinationType::VOLUME ? "volume" : "disk"), + move_ttl_entry->destination_name, + *std::atomic_load(&log_name)); + } else { LOG_TRACE(log, "Reserving bytes on selected destination"); @@ -4978,41 +4987,33 @@ ReservationPtr MergeTreeData::tryReserveSpacePreferringTTLRules( return reservation; } -SpacePtr MergeTreeData::getDestinationForMoveTTL(const TTLDescription & move_ttl, bool is_insert) const +SpacePtr MergeTreeData::getDestinationForMoveTTL(const TTLDescription & move_ttl) const { auto policy = getStoragePolicy(); if (move_ttl.destination_type == DataDestinationType::VOLUME) - { - auto volume = policy->tryGetVolumeByName(move_ttl.destination_name); - - if (!volume) - return {}; - - if (is_insert && !volume->perform_ttl_move_on_insert) - return {}; - - return volume; - } + return policy->tryGetVolumeByName(move_ttl.destination_name); else if (move_ttl.destination_type == DataDestinationType::DISK) - { - auto disk = policy->tryGetDiskByName(move_ttl.destination_name); - - if (!disk) - return {}; - - auto volume = policy->getVolume(policy->getVolumeIndexByDisk(disk)); - if (!volume) - return {}; - - if (is_insert && !volume->perform_ttl_move_on_insert) - return {}; - - return disk; - } + return policy->tryGetDiskByName(move_ttl.destination_name); else return {}; } +bool MergeTreeData::shouldPerformTTLMoveOnInsert(const SpacePtr & move_destination) const +{ + if (move_destination->isVolume()) + { + auto volume = std::static_pointer_cast(move_destination); + return volume->perform_ttl_move_on_insert; + } + if (move_destination->isDisk()) + { + auto disk = std::static_pointer_cast(move_destination); + if (auto volume = getStoragePolicy()->tryGetVolumeByDisk(disk)) + return volume->perform_ttl_move_on_insert; + } + return false; +} + bool MergeTreeData::isPartInTTLDestination(const TTLDescription & ttl, const IMergeTreeDataPart & part) const { auto policy = getStoragePolicy(); diff --git a/src/Storages/MergeTree/MergeTreeData.h b/src/Storages/MergeTree/MergeTreeData.h index 08523dba858..43358be6fd9 100644 --- a/src/Storages/MergeTree/MergeTreeData.h +++ b/src/Storages/MergeTree/MergeTreeData.h @@ -867,9 +867,12 @@ public: /// Return alter conversions for part which must be applied on fly. AlterConversions getAlterConversionsForPart(MergeTreeDataPartPtr part) const; - /// Returns destination disk or volume for the TTL rule according to current storage policy - /// 'is_insert' - is TTL move performed on new data part insert. - SpacePtr getDestinationForMoveTTL(const TTLDescription & move_ttl, bool is_insert = false) const; + + /// Returns destination disk or volume for the TTL rule according to current storage policy. + SpacePtr getDestinationForMoveTTL(const TTLDescription & move_ttl) const; + + /// Whether INSERT of a data part which is already expired should move it immediately to a volume/disk declared in move rule. + bool shouldPerformTTLMoveOnInsert(const SpacePtr & move_destination) const; /// Checks if given part already belongs destination disk or volume for the /// TTL rule.