feat(cloud): Add File Cache Consistency Check (#41280) #43440

Lupinus · 2024-11-07T09:58:04Z

Add a feature to verify the consistency of the file cache, checking whether the disk cache is under the control of the file cache management system.

What problem does this PR solve?

Occasionally, we found that there have been cases of disk cache data escaping from the management of Doris file cache, causing disk space leaks. To make it easier for debugging, we need a checking tool that compares the contents in the Doris file cache memory management structure with the current disk contents to identify the differences between the two (which are potential problematic data).

Check List (For Author)

Test
- Regression test
- Unit Test
- Manual test (add detailed scripts or steps below)
- No need to test or manual test. Explain why:
  - This is a refactor/code format and no logic has been changed.
  - Previous test can cover this change.
  - No colde files have been changed.
  - Other reason
Behavior changed:
- No.
- Yes.
Does this need documentation?
- No.
- Yes.
Release note

None

Check List (For Reviewer who merge this PR)

Confirm the release note
Confirm test cases
Confirm document
Add branch pick label

doris-robot · 2024-11-07T09:58:09Z

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

github-actions

clang-tidy made some suggestions

github-actions · 2024-11-07T10:02:53Z

be/src/io/cache/fs_file_cache_storage.cpp

@@ -588,6 +588,114 @@ void FSFileCacheStorage::load_cache_info_into_memory(BlockFileCache* _mgr) const
    TEST_SYNC_POINT_CALLBACK("BlockFileCache::TmpFile2");
 }

+void FSFileCacheStorage::check_consistency(


warning: function 'check_consistency' has cognitive complexity of 59 (threshold 50) [readability-function-cognitive-complexity]

void FSFileCacheStorage::check_consistency( ^

Additional context

be/src/io/cache/fs_file_cache_storage.cpp:596: nesting level increased to 1

auto check = [_mgr, &cache_lock, &confirmed_blocks, &inconsistent_cache_context, ^

be/src/io/cache/fs_file_cache_storage.cpp:598: +2, including nesting penalty of 1, nesting level increased to 2

for (; key_it != std::filesystem::directory_iterator(); ++key_it) { ^

be/src/io/cache/fs_file_cache_storage.cpp:607: +3, including nesting penalty of 2, nesting level increased to 3

if (ec) [[unlikely]] { ^

be/src/io/cache/fs_file_cache_storage.cpp:613: +3, including nesting penalty of 2, nesting level increased to 3

for (; offset_it != std::filesystem::directory_iterator(); ++offset_it) { ^

be/src/io/cache/fs_file_cache_storage.cpp:618: +4, including nesting penalty of 3, nesting level increased to 4

if (!this->parse_filename_suffix_to_cache_type( ^

be/src/io/cache/fs_file_cache_storage.cpp:627: +4, including nesting penalty of 3, nesting level increased to 4

if (!cell || cell->is_deleted) { ^

be/src/io/cache/fs_file_cache_storage.cpp:627: +1

if (!cell || cell->is_deleted) { ^

be/src/io/cache/fs_file_cache_storage.cpp:636: +4, including nesting penalty of 3, nesting level increased to 4

? cell->dowloading_size() ^

be/src/io/cache/fs_file_cache_storage.cpp:635: +1

(is_tmp && cell->file_block->state() == FileBlock::State::DOWNLOADING) ^

be/src/io/cache/fs_file_cache_storage.cpp:639: +4, including nesting penalty of 3, nesting level increased to 4

if (size != expected_size) { ^

be/src/io/cache/fs_file_cache_storage.cpp:643: +4, including nesting penalty of 3, nesting level increased to 4

if (cache_type != cell->file_block->cache_type()) { ^

be/src/io/cache/fs_file_cache_storage.cpp:647: +4, including nesting penalty of 3, nesting level increased to 4

if (expiration_time != cell->file_block->expiration_time()) { ^

be/src/io/cache/fs_file_cache_storage.cpp:652: +4, including nesting penalty of 3, nesting level increased to 4

if (inconsistent_type != InconsistentCacheContext::InconsistentType::NONE) { ^

be/src/io/cache/fs_file_cache_storage.cpp:659: +1, including nesting penalty of 0, nesting level increased to 1

if constexpr (USE_CACHE_VERSION2) { ^

be/src/io/cache/fs_file_cache_storage.cpp:661: +2, including nesting penalty of 1, nesting level increased to 2

if (ec) { ^

be/src/io/cache/fs_file_cache_storage.cpp:665: +2, including nesting penalty of 1, nesting level increased to 2

for (; key_prefix_it != std::filesystem::directory_iterator(); ++key_prefix_it) { ^

be/src/io/cache/fs_file_cache_storage.cpp:666: +3, including nesting penalty of 2, nesting level increased to 3

if (!key_prefix_it->is_directory()) { ^

be/src/io/cache/fs_file_cache_storage.cpp:670: +3, including nesting penalty of 2, nesting level increased to 3

if (key_prefix_it->path().filename().native().size() != KEY_PREFIX_LENGTH) { ^

be/src/io/cache/fs_file_cache_storage.cpp:675: +4, including nesting penalty of 3, nesting level increased to 4

if (ec) { ^

be/src/io/cache/fs_file_cache_storage.cpp:682: +3, including nesting penalty of 2, nesting level increased to 3

if (ec) { ^

be/src/io/cache/fs_file_cache_storage.cpp:688: +1, nesting level increased to 1

} else { ^

be/src/io/cache/fs_file_cache_storage.cpp:690: +2, including nesting penalty of 1, nesting level increased to 2

if (ec) { ^

github-actions · 2024-11-07T10:02:54Z

be/src/io/cache/fs_file_cache_storage.cpp

@@ -588,6 +588,114 @@
    TEST_SYNC_POINT_CALLBACK("BlockFileCache::TmpFile2");
 }

+void FSFileCacheStorage::check_consistency(


warning: function 'check_consistency' exceeds recommended size/complexity thresholds [readability-function-size]

void FSFileCacheStorage::check_consistency( ^

Additional context

be/src/io/cache/fs_file_cache_storage.cpp:590: 102 lines including whitespace and comments (threshold 80)

void FSFileCacheStorage::check_consistency( ^

github-actions · 2024-11-07T10:02:54Z

be/test/io/cache/block_file_cache_test.cpp

@@ -21,6 +21,7 @@
 #include <gen_cpp/Types_types.h>


warning: 'gen_cpp/Types_types.h' file not found [clang-diagnostic-error]

#include <gen_cpp/Types_types.h> ^

github-actions · 2024-11-07T10:02:54Z

be/test/io/cache/block_file_cache_test.cpp

@@ -5284,6 +5285,145 @@
    }
 }

+TEST_F(BlockFileCacheTest, check_fs_file_cache_consistency) {


warning: function 'TEST_F' exceeds recommended size/complexity thresholds [readability-function-size]

^

Additional context

be/test/io/cache/block_file_cache_test.cpp:5287: 137 lines including whitespace and comments (threshold 80)

^

gavinchou · 2024-11-07T10:43:18Z

be/src/io/cache/fs_file_cache_storage.h

@@ -102,6 +102,12 @@ class FSFileCacheStorage : public FileCacheStorage {

    void load_cache_info_into_memory(BlockFileCache* _mgr) const;

+    void check_consistency(
+            BlockFileCache* _mgr,


Better not couple this interface with FileCacheManager. The method of checking here is that the storage can return a list with comprehensive "file" information (including path, size, creation time, etc.), preferably in an ordered manner. Let the FileCacheManager, which depends on this storage, perform its own forward and reverse checks to see if there is any extra or missing data in the storage.

Thank you for the correction, I will make the modifications promptly.

freemandealer · 2024-11-08T08:17:03Z

run buildall

freemandealer

greaaat work! thanks for your contribution!

freemandealer · 2024-11-08T08:23:13Z

be/src/http/action/file_cache_action.cpp

@@ -50,6 +50,8 @@ constexpr static std::string_view CLEAR = "clear";
 constexpr static std::string_view RESET = "reset";
 constexpr static std::string_view HASH = "hash";
 constexpr static std::string_view LIST_CACHE = "list_cache";
+constexpr static std::string_view VIEW = "view";


list_base_paths might be more explicit

freemandealer · 2024-11-08T08:25:30Z

be/src/http/action/file_cache_action.cpp

+        *json_metrics = json_array.ToString();
+    } else if (operation == CHECK) {
+        const std::string& cache_base_path = req->param(BASE_PATH.data());
+        auto* block_file_cache = io::FileCacheFactory::instance()->get_by_path(cache_base_path);


handle the error if the path is invalid. otherwise this will crash into nullptr.

freemandealer · 2024-11-08T08:27:29Z

be/src/io/cache/block_file_cache.cpp

+std::vector<std::string> BlockFileCache::check_file_cache_consistency() {
+    std::unordered_set<AccessKeyAndOffset, KeyAndOffsetHash> confirmed_blocks;
+    std::vector<InconsistentCacheContext> inconsistent_cache_context;
+    std::lock_guard<std::mutex> lock(_mutex);


naming advice: lock -> cache_lock for efficient code search.

freemandealer · 2024-11-08T08:46:12Z

be/src/io/cache/file_cache_common.h

+        uint32_t type;
+
+    public:
+        enum : uint32_t {


very compact, that's nice...

freemandealer · 2024-11-08T08:47:47Z

be/src/io/cache/file_cache_common.h

+    public:
+        enum : uint32_t {
+            NONE = 0,
+            FILES_INCONSISTENT = 1 << 0,


the description for each type is a necessary here

freemandealer · 2024-11-08T08:57:24Z

be/src/io/cache/file_cache_common.h

+        enum : uint32_t {
+            NONE = 0,
+            FILES_INCONSISTENT = 1 << 0,
+            STORAGE_INCONSISTENT = 1 << 1,


"INCONSISTENT" is bidirectional. Might 'MISSING_IN_STORAGE' be more explicit?

freemandealer · 2024-11-08T09:05:09Z

be/src/io/cache/file_cache_common.cpp

+       << "Expiration Time: " << expiration_time << "\n"
+       << "Offset: " << offset << "\n"
+       << "Cache Type: " << file_cache_type_to_string(cache_type) << "\n"
+       << "Inconsistent Type: " << inconsistent_type.to_string();


freemandealer · 2024-11-08T09:39:00Z

be/src/io/cache/fs_file_cache_storage.cpp

+                FileCacheType cache_type = FileCacheType::NORMAL;
+                if (!this->parse_filename_suffix_to_cache_type(
+                            fs, offset_it->path().filename().native(), expiration_time, size,
+                            &offset, &is_tmp, &cache_type)) {


if is_tmp == true, the type will never be assigned (always be the default value FileCacheType::NORMAL). check the internal of parse_filename_suffix_to_cache_type for details.

maybe add TMP as another 'type' in file_cache_type_to_string(be/src/io/cache/file_cache_common.cpp ). As I see it, there is no counterpart in management structure in memory. Maybe we can deal with TMP like this:
if a tmp file with state 'DOWNLOADING', it's consistent. otherwise, inconsistent.

freemandealer · 2024-11-08T12:35:22Z

be/src/io/cache/fs_file_cache_storage.cpp

+                std::string offset_path = offset_it->path();
+                auto* cell = _mgr->get_cell(hash, offset, cache_lock);
+
+                if (!cell || cell->is_deleted) {


is_deleted works as a flag, the file is not deleted yet

freemandealer · 2024-11-08T12:39:28Z

be/src/io/cache/fs_file_cache_storage.cpp

+                continue;
+            }
+            if (key_prefix_it->path().filename().native().size() != KEY_PREFIX_LENGTH) {
+                LOG(WARNING) << "Unknown directory " << key_prefix_it->path().native()


no deletion here. this is just a checking tool, don't touch anything!

freemandealer · 2024-11-08T12:41:24Z

be/src/io/cache/fs_file_cache_storage.cpp

+            check(key_it);
+        }
+    } else {
+        std::filesystem::directory_iterator key_it {_cache_base_path, ec};


are you sure this check works for both VERSION1 and VERSION2? if not, just warning here "only support version2"

freemandealer · 2024-11-08T12:46:57Z

be/src/io/cache/block_file_cache.cpp

+    _storage->check_consistency(this, confirmed_blocks, inconsistent_cache_context, lock);
+    for (const auto& [hash, offset_to_cell] : _files) {
+        for (const auto& [offset, cell] : offset_to_cell) {
+            if (cell.is_deleted || confirmed_blocks.contains({hash, offset})) {


is_deleted works as a flag, the file is not deleted yet

freemandealer · 2024-11-08T12:48:12Z

be/src/io/cache/file_cache_common.h

+    public:
+        enum : uint32_t {
+            NONE = 0,
+            FILES_INCONSISTENT = 1 << 0,


may be 'NOT_LOADED'?

freemandealer · 2024-11-08T12:54:41Z

be/test/io/cache/block_file_cache_test.cpp

+        assert_range(2, blocks[0], io::FileBlock::Range(10, 18), io::FileBlock::State::DOWNLOADING);
+        download(blocks[0]);
+        mgr._files[key1].erase(10);
+        mgr.check_file_cache_consistency();


unnecessary

That's for DEBUG, forgot to remove :D

freemandealer · 2024-11-08T12:54:47Z

be/test/io/cache/block_file_cache_test.cpp

+        std::string dir_path = fs_file_cache_storage->get_path_in_local_cache(key1, 0);
+        fs::path block_file_path = std::filesystem::path(dir_path) / "20";
+        fs::remove(block_file_path);
+        mgr.check_file_cache_consistency();


freemandealer · 2024-11-08T12:54:52Z

be/test/io/cache/block_file_cache_test.cpp

+        std::ofstream out_file(block_file_path, std::ios::out | std::ios::app);
+        out_file << data;
+        out_file.close();
+        mgr.check_file_cache_consistency();


freemandealer · 2024-11-08T12:54:57Z

be/test/io/cache/block_file_cache_test.cpp

+        assert_range(2, blocks[0], io::FileBlock::Range(40, 48), io::FileBlock::State::DOWNLOADING);
+        download(blocks[0]);
+        blocks[0]->_key.meta.type = io::FileCacheType::INDEX;
+        mgr.check_file_cache_consistency();


freemandealer · 2024-11-08T12:55:03Z

be/test/io/cache/block_file_cache_test.cpp

+        assert_range(2, blocks[0], io::FileBlock::Range(0, 8), io::FileBlock::State::DOWNLOADING);
+        download(blocks[0]);
+        blocks[0]->_key.meta.expiration_time = 0;
+        mgr.check_file_cache_consistency();


github-actions

clang-tidy made some suggestions

github-actions · 2024-11-12T15:43:51Z

be/test/io/cache/block_file_cache_test.cpp

@@ -5284,6 +5285,155 @@ TEST_F(BlockFileCacheTest, file_cache_path_storage_parse) {
    }
 }

+TEST_F(BlockFileCacheTest, check_fs_file_cache_consistency) {


warning: function 'TEST_F' exceeds recommended size/complexity thresholds [readability-function-size]

^

Additional context

be/test/io/cache/block_file_cache_test.cpp:5287: 147 lines including whitespace and comments (threshold 80)

^

Lupinus · 2024-11-13T07:39:31Z

run buildall

Lupinus · 2024-11-21T13:09:21Z

run buildall

freemandealer

The UT failed in CI: http://43.132.222.7:8111/buildConfiguration/Doris_DorisBeUt_BeUt/572816?expandBuildDeploymentsSection=false&hideTestsFromDependencies=false&hideProblemsFromDependencies=false&expandPull+Request+Details=true&expandBuildProblemsSection=true&expandBuildTestsSection=true&expandBuildChangesSection=true

freemandealer · 2024-11-25T04:14:26Z

be/src/http/action/file_cache_action.cpp

+        if (cache_base_path.empty()) {
+            st = Status::InvalidArgument("missing parameter: {} is required", VALUE.data());
+        } else {
+            auto* block_file_cache = io::FileCacheFactory::instance()->get_by_path(cache_base_path);


if (!block_file_cache) {
st = Status::InvalidArgument("invalid path: {}", VALUE.data());
return st;
}

freemandealer · 2024-11-25T06:42:20Z

run external

Add a feature to verify the consistency of the file cache, checking whether the disk cache is under the control of the file cache management system.

Lupinus · 2024-12-05T05:26:33Z

run buildall

doris-robot · 2024-12-05T06:54:32Z

TeamCity be ut coverage result:
Function Coverage: 38.48% (10015/26025)
Line Coverage: 29.56% (84077/284459)
Region Coverage: 28.66% (43206/150758)
Branch Coverage: 25.26% (21978/86990)
Coverage Report: http://coverage.selectdb-in.cc/coverage/84a990d31626571d7dcb2646d2ce9243de9391c7_84a990d31626571d7dcb2646d2ce9243de9391c7/report/index.html

Lupinus · 2024-12-05T09:41:41Z

run buildall

doris-robot · 2024-12-05T11:11:28Z

TeamCity be ut coverage result:
Function Coverage: 38.49% (10014/26015)
Line Coverage: 29.55% (84069/284497)
Region Coverage: 28.65% (43201/150788)
Branch Coverage: 25.25% (21973/87020)
Coverage Report: http://coverage.selectdb-in.cc/coverage/84a990d31626571d7dcb2646d2ce9243de9391c7_84a990d31626571d7dcb2646d2ce9243de9391c7/report/index.html

Lupinus · 2024-12-07T10:41:49Z

run p0

Lupinus · 2024-12-15T07:11:41Z

run buildall

doris-robot · 2024-12-15T08:53:05Z

TeamCity be ut coverage result:
Function Coverage: 38.77% (10116/26094)
Line Coverage: 29.73% (85028/285997)
Region Coverage: 28.79% (43638/151559)
Branch Coverage: 25.36% (22190/87498)
Coverage Report: http://coverage.selectdb-in.cc/coverage/84a990d31626571d7dcb2646d2ce9243de9391c7_84a990d31626571d7dcb2646d2ce9243de9391c7/report/index.html

github-actions bot reviewed Nov 7, 2024

View reviewed changes

gavinchou requested changes Nov 7, 2024

View reviewed changes

freemandealer suggested changes Nov 8, 2024

View reviewed changes

freemandealer reviewed Nov 8, 2024

View reviewed changes

Lupinus force-pushed the issue#41280 branch from 17369e8 to f7c7869 Compare November 12, 2024 15:38

github-actions bot reviewed Nov 12, 2024

View reviewed changes

Lupinus force-pushed the issue#41280 branch 2 times, most recently from b2e2c54 to 590dffa Compare November 13, 2024 07:37

Lupinus force-pushed the issue#41280 branch from 590dffa to 7db7a4e Compare November 21, 2024 13:07

freemandealer suggested changes Nov 25, 2024

View reviewed changes

feat(cloud): Add File Cache Consistency Check (apache#41280)

84a990d

Add a feature to verify the consistency of the file cache, checking whether the disk cache is under the control of the file cache management system.

Lupinus force-pushed the issue#41280 branch from 7db7a4e to 84a990d Compare December 5, 2024 05:23

feat(cloud): Add File Cache Consistency Check (#41280) #43440

Are you sure you want to change the base?

feat(cloud): Add File Cache Consistency Check (#41280) #43440

Conversation

Lupinus commented Nov 7, 2024 • edited Loading

What problem does this PR solve?

Check List (For Author)

Check List (For Reviewer who merge this PR)

doris-robot commented Nov 7, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Nov 7, 2024

Choose a reason for hiding this comment

github-actions bot Nov 7, 2024

Choose a reason for hiding this comment

github-actions bot Nov 7, 2024

Choose a reason for hiding this comment

github-actions bot Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freemandealer commented Nov 8, 2024

freemandealer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freemandealer Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freemandealer Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Nov 12, 2024

Choose a reason for hiding this comment

Lupinus commented Nov 13, 2024

Lupinus commented Nov 21, 2024

freemandealer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freemandealer commented Nov 25, 2024

Lupinus commented Dec 5, 2024

doris-robot commented Dec 5, 2024

Lupinus commented Dec 5, 2024

doris-robot commented Dec 5, 2024

Lupinus commented Dec 7, 2024

Lupinus commented Dec 15, 2024

doris-robot commented Dec 15, 2024

Lupinus commented Nov 7, 2024 •

edited

Loading

freemandealer Nov 8, 2024 •

edited

Loading

freemandealer Nov 8, 2024 •

edited

Loading