Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Catch exceptions from create_directories in fs cache #49203

Merged
merged 1 commit into from Apr 27, 2023
Merged

Catch exceptions from create_directories in fs cache #49203

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
70 changes: 38 additions & 32 deletions src/Interpreters/Cache/FileCache.cpp
View file
Open in desktop
Expand Up @@ -497,62 +497,68 @@ FileCache::FileSegmentCell * FileCache::addCell(
/// Create a file segment cell and put it in `files` map by [key][offset].

if (!size)
return nullptr; /// Empty files are not cached.
throw Exception(ErrorCodes::LOGICAL_ERROR, "Zero size files are not allowed");

if (files[key].contains(offset))
throw Exception(
ErrorCodes::LOGICAL_ERROR,
"Cache cell already exists for key: `{}`, offset: {}, size: {}.\nCurrent cache structure: {}",
key.toString(), offset, size, dumpStructureUnlocked(key, cache_lock));

auto skip_or_download = [&]() -> FileSegmentPtr
FileSegment::State result_state = state;
if (state == FileSegment::State::EMPTY && enable_cache_hits_threshold)
{
FileSegment::State result_state = state;
if (state == FileSegment::State::EMPTY && enable_cache_hits_threshold)
auto record = stash_records.find({key, offset});

if (record == stash_records.end())
{
auto record = stash_records.find({key, offset});
auto priority_iter = stash_priority->add(key, offset, 0, cache_lock);
stash_records.insert({{key, offset}, priority_iter});

if (record == stash_records.end())
if (stash_priority->getElementsNum(cache_lock) > max_stash_element_size)
{
auto priority_iter = stash_priority->add(key, offset, 0, cache_lock);
stash_records.insert({{key, offset}, priority_iter});

if (stash_priority->getElementsNum(cache_lock) > max_stash_element_size)
{
auto remove_priority_iter = stash_priority->getLowestPriorityWriteIterator(cache_lock);
stash_records.erase({remove_priority_iter->key(), remove_priority_iter->offset()});
remove_priority_iter->removeAndGetNext(cache_lock);
}

/// For segments that do not reach the download threshold,
/// we do not download them, but directly read them
result_state = FileSegment::State::SKIP_CACHE;
auto remove_priority_iter = stash_priority->getLowestPriorityWriteIterator(cache_lock);
stash_records.erase({remove_priority_iter->key(), remove_priority_iter->offset()});
remove_priority_iter->removeAndGetNext(cache_lock);
}
else
{
auto priority_iter = record->second;
priority_iter->use(cache_lock);

result_state = priority_iter->hits() >= enable_cache_hits_threshold
? FileSegment::State::EMPTY
: FileSegment::State::SKIP_CACHE;
}
/// For segments that do not reach the download threshold,
/// we do not download them, but directly read them
result_state = FileSegment::State::SKIP_CACHE;
}
else
{
auto priority_iter = record->second;
priority_iter->use(cache_lock);

return std::make_shared<FileSegment>(offset, size, key, this, result_state, settings);
};
result_state = priority_iter->hits() >= enable_cache_hits_threshold
? FileSegment::State::EMPTY
: FileSegment::State::SKIP_CACHE;
}
}

FileSegmentCell cell(skip_or_download(), this, cache_lock);
auto & offsets = files[key];

if (offsets.empty())
{
auto key_path = getPathInLocalCache(key);

if (!fs::exists(key_path))
fs::create_directories(key_path);
{
try
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to add a comment about the possible reasons - why sometimes creating a directory fails and why we should catch the exception here instead of letting it propagate and be caught in the upper layer.

Copy link
Member Author

@kssenii kssenii Apr 27, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will add a comment, the initial reason for this PR was that create_directories can fail with "no space left on device". And it is less convenient to propagate exception to the upper level, because we want to skip cache in this case, which should be done here by assigning a file segment the state SKIP_CACHE, which was done in this PR.

 Received exception from server (version 23.3.1):
Code: 1001. DB::Exception: Received from . DB::Exception: Received from . DB::Exception: std::__1::__fs::filesystem::filesystem_error: filesystem error: in create_directories: No space left on device ["/mnt/clickhouse-cache/s3disk/hash”]. Stack trace:

0. std::system_error::system_error(std::error_code, String const&) @ 0x1a417012 in /usr/bin/clickhouse
1. ? @ 0xe100a63 in /usr/bin/clickhouse
2. ? @ 0x1a3bc6da in /usr/bin/clickhouse
3. ? @ 0x1a3bfe36 in /usr/bin/clickhouse
4. std::__fs::filesystem::__create_directories(std::__fs::filesystem::path const&, std::error_code*) @ 0x1a3c0669 in /usr/bin/clickhouse
5. DB::FileCache::addCell(DB::FileCacheKey const&, unsigned long, unsigned long, DB::FileSegment::State, DB::CreateFileSegmentSettings const&, std::lock_guard<std::mutex>&) @ 0x13a4e300 in /usr/bin/clickhouse
6. DB::FileCache::getOrSet(DB::FileCacheKey const&, unsigned long, unsigned long, DB::CreateFileSegmentSettings const&) @ 0x13a4f14c in /usr/bin/clickhouse

{
fs::create_directories(key_path);
}
catch (...)
{
tryLogCurrentException(__PRETTY_FUNCTION__);
result_state = FileSegment::State::SKIP_CACHE;
}
}
}

auto file_segment = std::make_shared<FileSegment>(offset, size, key, this, result_state, settings);
FileSegmentCell cell(std::move(file_segment), this, cache_lock);

auto [it, inserted] = offsets.insert({offset, std::move(cell)});
if (!inserted)
throw Exception(
Expand Down