Disagg: fix used size metrics of FileCache (#8921) (#8923)

close #8920
pingcap · Apr 10, 2024 · e844048 · e844048
1 parent dde6a2a
commit e844048
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 11 deletions.
diff --git a/dbms/src/Storages/S3/FileCache.cpp b/dbms/src/Storages/S3/FileCache.cpp
@@ -166,7 +166,7 @@ FileSegmentPtr FileCache::get(const S3::S3FilenameView & s3_fname, const std::op
 }
 
 // Remove `local_fname` from disk and remove parent directory if parent directory is empty.
-void FileCache::removeDiskFile(const String & local_fname)
+void FileCache::removeDiskFile(const String & local_fname, bool update_fsize_metrics) const
 {
     if (!std::filesystem::exists(local_fname))
     {
@@ -181,9 +181,7 @@ void FileCache::removeDiskFile(const String & local_fname)
             if (s != cache_dir && (s == local_fname || std::filesystem::is_empty(p)))
             {
                 std::filesystem::remove(p); // If p is a directory, remove success only when it is empty.
-                // Temporary files are not reported size to metrics until they are renamed.
-                // So we don't need to free its size here.
-                if (s == local_fname && !isTemporaryFilename(local_fname))
+                if (s == local_fname && update_fsize_metrics)
                 {
                     capacity_metrics->freeUsedSize(local_fname, fsize);
                 }
@@ -231,9 +229,10 @@ std::pair<Int64, std::list<String>::iterator> FileCache::removeImpl(
         return {-1, {}};
     }
     const auto & local_fname = f->getLocalFileName();
-    removeDiskFile(local_fname);
+    removeDiskFile(local_fname, /*update_fsize_metrics*/ true);
     auto temp_fname = toTemporaryFilename(local_fname);
-    removeDiskFile(temp_fname);
+    // Not update fsize metrics for temporary files because they are not add to fsize metrics before.
+    removeDiskFile(temp_fname, /*update_fsize_metrics*/ false);
 
     auto release_size = f->getSize();
     GET_METRIC(tiflash_storage_remote_cache, type_dtfile_evict).Increment();
@@ -651,7 +650,8 @@ void FileCache::restoreDMFile(const std::filesystem::directory_entry & dmfile_en
         auto fname = file_entry.path().string();
         if (unlikely(isTemporaryFilename(fname)))
         {
-            removeDiskFile(fname);
+            // Not update fsize metrics for temporary files because they are not add to fsize metrics before.
+            removeDiskFile(fname, /*update_fsize_metrics*/ false);
         }
         else
         {
@@ -669,7 +669,8 @@ void FileCache::restoreDMFile(const std::filesystem::directory_entry & dmfile_en
             }
             else
             {
-                removeDiskFile(fname);
+                // Not update fsize metrics because this file is not added to metrics yet.
+                removeDiskFile(fname, /*update_fsize_metrics*/ false);
             }
         }
     }

diff --git a/dbms/src/Storages/S3/FileCache.h b/dbms/src/Storages/S3/FileCache.h
@@ -256,7 +256,7 @@ class FileCache
         const String & s3_key,
         FileSegmentPtr & f,
         bool force = false);
-    void removeDiskFile(const String & local_fname);
+    void removeDiskFile(const String & local_fname, bool update_fsize_metrics) const;
 
     // Estimated size is an empirical value.
     // We don't know object size before get object from S3.

diff --git a/dbms/src/Storages/S3/tests/gtest_filecache.cpp b/dbms/src/Storages/S3/tests/gtest_filecache.cpp
@@ -379,7 +379,7 @@ TEST_F(FileCacheTest, FileSystem)
     }
     ASSERT_TRUE(std::filesystem::exists(local_file2));
 
-    file_cache.removeDiskFile(local_fname1);
+    file_cache.removeDiskFile(local_fname1, false);
     ASSERT_FALSE(std::filesystem::exists(local_file1)) << local_file1.generic_string();
     ASSERT_TRUE(std::filesystem::exists(local_file2)) << local_file2.generic_string();
     ASSERT_TRUE(std::filesystem::exists(dmf)) << dmf.generic_string();
@@ -393,7 +393,7 @@ TEST_F(FileCacheTest, FileSystem)
     ASSERT_TRUE(std::filesystem::exists(cache_root)) << cache_root.generic_string();
     ASSERT_EQ(cache_root.generic_string(), cache_config.getDTFileCacheDir());
 
-    file_cache.removeDiskFile(local_fname2);
+    file_cache.removeDiskFile(local_fname2, false);
     ASSERT_FALSE(std::filesystem::exists(local_file2)) << local_file2.generic_string();
     ASSERT_FALSE(std::filesystem::exists(dmf)) << dmf.generic_string();
     ASSERT_FALSE(std::filesystem::exists(table)) << table.generic_string();