Skip to content

Commit

Permalink
[BugFix] Format the datacache path configurations before checking it …
Browse files Browse the repository at this point in the history
…instead of returning error directly.(backport #41921) (#42028)

Signed-off-by: Gavin <[email protected]>
  • Loading branch information
GavinMar authored Mar 4, 2024
1 parent 3c5535a commit e1c6e4e
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 79 deletions.
18 changes: 0 additions & 18 deletions be/src/block_cache/block_cache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down
25 changes: 15 additions & 10 deletions be/src/storage/options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 << "]";
Expand Down
80 changes: 29 additions & 51 deletions be/test/block_cache/block_cache_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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";
Expand Down

0 comments on commit e1c6e4e

Please sign in to comment.