[enhancement](cloud) file cache evict in advance #47473
Conversation
evict in advance if current cache size is over threshold to avoid sync evict during query, which may affect query performance. Signed-off-by: zhengyu <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
|
TeamCity be ut coverage result: |
|
run buildall |
|
run performance |
|
TeamCity be ut coverage result: |
Signed-off-by: zhengyu <[email protected]>
freemandealer
dismissed stale reviews from dataroaring and gavinchou
via
0a01d0f
|
run buildall |
github-actions
bot
removed
the
approved
TPC-H: Total hot run time: 32452 ms |
TPC-DS: Total hot run time: 191066 ms |
ClickBench: Total hot run time: 30.18 s |
|
TeamCity be ut coverage result: |
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
| @@ -186,6 +189,9 @@ class BlockFileCache { | |||
| bool try_reserve(const UInt128Wrapper& hash, const CacheContext& context, size_t offset, | |||
| size_t size, std::lock_guard<std::mutex>& cache_lock); | |||
|
|
|||
| // async evict when water mark is reached, unlike try_reserve, removal is done in background | |||
| void try_evict_in_advance(size_t size, std::lock_guard<std::mutex>& cache_lock); | |||
There was a problem hiding this comment.
add detailed comment what is the water mark, what is the behavor/machanism of this function, what is the assumption
| @@ -1756,6 +1832,32 @@ void BlockFileCache::run_background_gc() { | |||
| } | |||
| } | |||
|
|
|||
| void BlockFileCache::run_background_evict_in_advance() { | |||
There was a problem hiding this comment.
add log when this thread starts and exits and reason.
| { | ||
| SCOPED_CACHE_LOCK(_mutex); | ||
| SCOPED_RAW_TIMER(&duration_ns); | ||
| try_evict_in_advance(batch, cache_lock); |
There was a problem hiding this comment.
add bvar for observing bytes reserved by background thread
| @@ -1648,11 +1666,69 @@ void BlockFileCache::check_disk_resource_limit() { | |||
| } | |||
| } | |||
|
|
|||
| void BlockFileCache::check_need_evict_cache_in_advance() { | |||
There was a problem hiding this comment.
add UT for this function, all branches should be tested
| } | ||
|
|
||
| std::pair<int, int> percent; | ||
| int ret = disk_used_percentage(_cache_base_path, &percent); |
There was a problem hiding this comment.
it may not work if the disk is not fully used by doris, e.g. we set file_cache capacity to 100GB how ever the disk capacity is 1000GB.
There was a problem hiding this comment.
The latter code in line #1680 will do the job.
| if (config::file_cache_enter_need_evict_cache_in_advance_percent <= | ||
| config::file_cache_exit_need_evict_cache_in_advance_percent) { | ||
| LOG_WARNING("config error, set to default value") | ||
| .tag("enter", config::file_cache_enter_need_evict_cache_in_advance_percent) | ||
| .tag("exit", config::file_cache_exit_need_evict_cache_in_advance_percent); | ||
| config::file_cache_enter_need_evict_cache_in_advance_percent = 78; |
There was a problem hiding this comment.
file_cache_enter_need_evict_cache_in_advance_percent and file_cache_exit_need_evict_cache_in_advance_percent are not a standalone config pair, they are affected by file_cache_enter_resource_limit_mode_percent
There was a problem hiding this comment.
why do you say that?
| DEFINE_mInt32(file_cache_enter_need_evict_cache_in_advance_percent, "88"); | ||
| DEFINE_mInt32(file_cache_exit_need_evict_cache_in_advance_percent, "80"); |
There was a problem hiding this comment.
88 and 80 are not consistent with the "default" values in function BlockFileCache::check_need_evict_cache_in_advance()
| DEFINE_mInt32(file_cache_enter_need_evict_cache_in_advance_percent, "88"); | ||
| DEFINE_mInt32(file_cache_exit_need_evict_cache_in_advance_percent, "80"); | ||
| DEFINE_mInt32(file_cache_evict_in_advance_interval_ms, "1000"); | ||
| DEFINE_mInt64(file_cache_evict_in_advance_batch_bytes, "31457280"); // 30MB |
There was a problem hiding this comment.
Too may handles for the functionality "evict in advance", e.g. what if we set file_cache_enter_need_evict_cache_in_advance_percent with a number larger than 100, is enable_evict_file_cache_in_advance still needed?
| ASSERT_EQ(cache.get_stats_unsafe()["index_queue_curr_size"], 0); | ||
| ASSERT_EQ(cache.get_stats_unsafe()["normal_queue_curr_size"], cache_max - 200000); | ||
|
|
||
| if (fs::exists(cache_base_path)) { |
There was a problem hiding this comment.
add more tests on the threshold for "evict in advance"
There was a problem hiding this comment.
sure, it will take some time
evict in advance if current cache size is over threshold to avoid sync evict during query, which may affect query performance.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)