[BugFix] Format the datacache path configurations before checking it …

…instead of returning error directly.(backport #41921) (#42028) Signed-off-by: Gavin <yangguansuo@starrocks.com>
StarRocks · Mar 4, 2024 · e1c6e4e · e1c6e4e
1 parent 3c5535a
commit e1c6e4e
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 79 deletions.
diff --git a/be/src/block_cache/block_cache.cpp b/be/src/block_cache/block_cache.cpp
@@ -45,24 +45,6 @@ BlockCache* BlockCache::instance() {
 }
 
 Status BlockCache::init(const CacheOptions& options) {
-    for (auto& dir : options.disk_spaces) {
-        if (dir.size == 0) {
-            continue;
-        }
-        fs::path dir_path(dir.path);
-        if (fs::exists(dir_path)) {
-            if (!fs::is_directory(dir_path)) {
-                LOG(ERROR) << "the block cache disk path already exists but not a directory, path: " << dir.path;
-                return Status::InvalidArgument("invalid block cache disk path");
-            }
-        } else {
-            std::error_code ec;
-            if (!fs::create_directory(dir_path, ec)) {
-                LOG(ERROR) << "create block cache disk path failed, path: " << dir.path << ", reason: " << ec.message();
-                return Status::InvalidArgument("invalid block cache disk path");
-            }
-        }
-    }
     _block_size = std::min(options.block_size, MAX_BLOCK_SIZE);
 #ifdef WITH_CACHELIB
     if (options.engine == "cachelib") {

diff --git a/be/src/storage/options.cpp b/be/src/storage/options.cpp
@@ -166,21 +166,26 @@ Status parse_conf_block_cache_paths(const std::string& config_path, std::vector<
     }
     std::vector<string> path_vec = strings::Split(config_path, ";", strings::SkipWhitespace());
     for (auto& item : path_vec) {
-        if (item.empty()) {
+        StripWhiteSpace(&item);
+        item.erase(item.find_last_not_of('/') + 1);
+        if (item.empty() || item[0] != '/') {
+            LOG(WARNING) << "invalid datacache path. path=" << item;
             continue;
         }
-        // Remove last slash if it exists$
-        auto it = item.end() - 1;
-        if (*it == '/') {
-            item.erase(it);
+
+        Status status = FileSystem::Default()->create_dir_if_missing(item);
+        if (!status.ok()) {
+            LOG(WARNING) << "datacache path can not be created. path=" << item;
+            continue;
         }
-        // Check the parent path
-        std::filesystem::path local_path(item);
-        if (local_path.has_parent_path() && !std::filesystem::exists(local_path.parent_path())) {
-            LOG(WARNING) << "invalid block cache path. path=" << item;
+
+        string canonicalized_path;
+        status = FileSystem::Default()->canonicalize(item, &canonicalized_path);
+        if (!status.ok()) {
+            LOG(WARNING) << "datacache path can not be canonicalized. may be not exist. path=" << item;
             continue;
         }
-        paths->emplace_back(local_path.string());
+        paths->emplace_back(canonicalized_path);
     }
     if ((path_vec.size() != paths->size() && !config::ignore_broken_disk)) {
         LOG(WARNING) << "fail to parse block_cache_disk_path config. value=[" << config_path << "]";

diff --git a/be/test/block_cache/block_cache_test.cpp b/be/test/block_cache/block_cache_test.cpp
@@ -14,76 +14,31 @@
 
 #include "block_cache/block_cache.h"
 
+#include <fmt/format.h>
 #include <gtest/gtest.h>
 
 #include <cstring>
+#include <filesystem>
 
 #include "common/logging.h"
 #include "common/statusor.h"
 #include "fs/fs_util.h"
+#include "storage/options.h"
 
 namespace starrocks {
 
 static const size_t block_size = 1024 * 1024;
 
 class BlockCacheTest : public ::testing::Test {
 protected:
-    static void SetUpTestCase() { ASSERT_TRUE(fs::create_directories("./ut_dir/block_disk_cache").ok()); }
+    static void SetUpTestCase() { ASSERT_TRUE(fs::create_directories("./block_disk_cache").ok()); }
 
-    static void TearDownTestCase() { ASSERT_TRUE(fs::remove_all("./ut_dir").ok()); }
+    static void TearDownTestCase() { ASSERT_TRUE(fs::remove_all("./block_disk_cache").ok()); }
 
     void SetUp() override {}
     void TearDown() override {}
 };
 
-TEST_F(BlockCacheTest, auto_create_disk_cache_path) {
-    std::unique_ptr<BlockCache> cache(new BlockCache);
-    const size_t block_size = 1024 * 1024;
-
-    CacheOptions options;
-    options.mem_space_size = 20 * 1024 * 1024;
-    size_t quota = 500 * 1024 * 1024;
-    options.disk_spaces.push_back({.path = "./ut_dir/final_entry_not_exist", .size = quota});
-    options.block_size = block_size;
-    options.max_concurrent_inserts = 100000;
-    options.enable_checksum = false;
-#ifdef WITH_STARCACHE
-    options.engine = "starcache";
-#else
-    options.engine = "cachelib";
-#endif
-    Status status = cache->init(options);
-    ASSERT_TRUE(status.ok());
-
-    const size_t batch_size = block_size - 1234;
-    const size_t rounds = 3;
-    const std::string cache_key = "test_file";
-
-    // write cache
-    off_t offset = 0;
-    for (size_t i = 0; i < rounds; ++i) {
-        char ch = 'a' + i % 26;
-        std::string value(batch_size, ch);
-        Status st = cache->write_cache(cache_key + std::to_string(i), 0, batch_size, value.c_str());
-        ASSERT_TRUE(st.ok());
-        offset += batch_size;
-    }
-
-    // read cache
-    offset = 0;
-    for (size_t i = 0; i < rounds; ++i) {
-        char ch = 'a' + i % 26;
-        std::string expect_value(batch_size, ch);
-        char value[batch_size] = {0};
-        auto res = cache->read_cache(cache_key + std::to_string(i), 0, batch_size, value);
-        ASSERT_TRUE(res.status().ok());
-        ASSERT_EQ(memcmp(value, expect_value.c_str(), batch_size), 0);
-        offset += batch_size;
-    }
-
-    cache->shutdown();
-}
-
 TEST_F(BlockCacheTest, copy_to_iobuf) {
     // Create an iobuffer which contains 3 blocks
     const size_t buf_block_size = 100;
@@ -112,6 +67,29 @@ TEST_F(BlockCacheTest, copy_to_iobuf) {
     ASSERT_EQ(memcmp(result, expect, size), 0);
 }
 
+TEST_F(BlockCacheTest, parse_cache_space_paths) {
+    const std::string cwd = std::filesystem::current_path().string();
+    const std::string s_normal_path = fmt::format("{}/block_disk_cache/cache1;{}/block_disk_cache/cache2", cwd, cwd);
+    std::vector<std::string> paths;
+    ASSERT_TRUE(parse_conf_block_cache_paths(s_normal_path, &paths).ok());
+    ASSERT_EQ(paths.size(), 2);
+
+    paths.clear();
+    const std::string s_space_path = fmt::format(" {}/block_disk_cache/cache3 ; {}/block_disk_cache/cache4 ", cwd, cwd);
+    ASSERT_TRUE(parse_conf_block_cache_paths(s_space_path, &paths).ok());
+    ASSERT_EQ(paths.size(), 2);
+
+    paths.clear();
+    const std::string s_empty_path = fmt::format("//;{}/block_disk_cache/cache4 ", cwd, cwd);
+    ASSERT_FALSE(parse_conf_block_cache_paths(s_empty_path, &paths).ok());
+    ASSERT_EQ(paths.size(), 1);
+
+    paths.clear();
+    const std::string s_invalid_path = fmt::format(" /block_disk_cache/cache5;{}/+/cache6", cwd, cwd);
+    ASSERT_FALSE(parse_conf_block_cache_paths(s_invalid_path, &paths).ok());
+    ASSERT_EQ(paths.size(), 0);
+}
+
 #ifdef WITH_STARCACHE
 TEST_F(BlockCacheTest, hybrid_cache) {
     std::unique_ptr<BlockCache> cache(new BlockCache);
@@ -120,7 +98,7 @@ TEST_F(BlockCacheTest, hybrid_cache) {
     CacheOptions options;
     options.mem_space_size = 10 * 1024 * 1024;
     size_t quota = 500 * 1024 * 1024;
-    options.disk_spaces.push_back({.path = "./ut_dir/block_disk_cache", .size = quota});
+    options.disk_spaces.push_back({.path = "./block_disk_cache", .size = quota});
     options.block_size = block_size;
     options.max_concurrent_inserts = 100000;
     options.engine = "starcache";