Merge pull request #41980 from vitlibar/remove-warning-when-perform-ttl-move-on-insert-false

Remove confusing warning when perform_ttl_move_on_insert is false

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.

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;

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;

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)

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;
 

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<IVolume>(move_destination);
+        return volume->perform_ttl_move_on_insert;
+    }
+    if (move_destination->isDisk())
+    {
+        auto disk = std::static_pointer_cast<IDisk>(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();

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.