freemandealer commented on code in PR #55772:
URL: https://github.com/apache/doris/pull/55772#discussion_r2385991359
##########
gensrc/thrift/PaloInternalService.thrift:
##########
@@ -408,6 +408,7 @@ struct TQueryOptions {
// In write path, to control if the content would be written into file cache.
// In read path, read from file cache or remote storage when execute query.
1000: optional bool disable_file_cache = false
+ 1001: optional bool enable_file_cache_query_limit = true
Review Comment:
正湖，你觉得我们把 file_cache_query_limit_bytes 也做成 session variable 怎么样？ 因为考虑到每个
query 对 cache 空间的需求天差地别，给一个接口让用户按需设置会好一些？
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -1941,6 +1904,13 @@ void BlockFileCache::run_background_monitor() {
(double)_num_read_blocks_1h->get_value());
}
}
+
+ if (config::file_cache_query_limit_bytes != 0 &&
+ config::file_cache_query_limit_bytes != _max_query_cache_size) {
+ LOG(INFO) << "file_cache_query_limit_bytes(" <<
config::file_cache_query_limit_bytes
Review Comment:
如果采用 TQueryOptions 每次把 limit bytes 带下来，也不用折腾这些更新了
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -301,6 +316,8 @@ BlockFileCache::QueryFileCacheContextPtr
BlockFileCache::get_or_set_query_contex
return nullptr;
}
+ LOG(INFO) << "BlockFileCache::get_or_set_query_context called for
query_id: "
Review Comment:
DEBUG 日志，删掉或者改用 VLOG
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -1018,73 +1035,19 @@ bool BlockFileCache::try_reserve(const UInt128Wrapper&
hash, const CacheContext&
int64_t cur_time = std::chrono::duration_cast<std::chrono::seconds>(
std::chrono::steady_clock::now().time_since_epoch())
.count();
- auto& queue = get_queue(context.cache_type);
- size_t removed_size = 0;
- size_t ghost_remove_size = 0;
- size_t queue_size = queue.get_capacity(cache_lock);
- size_t cur_cache_size = _cur_cache_size;
- size_t query_context_cache_size =
query_context->get_cache_size(cache_lock);
-
- std::vector<LRUQueue::Iterator> ghost;
- std::vector<FileBlockCell*> to_evict;
-
- size_t max_size = queue.get_max_size();
- auto is_overflow = [&] {
- return _disk_resource_limit_mode ? removed_size < size
- : cur_cache_size + size -
removed_size > _capacity ||
- (queue_size + size -
removed_size > max_size) ||
- (query_context_cache_size +
size -
- (removed_size +
ghost_remove_size) >
-
query_context->get_max_cache_size());
- };
-
- /// Select the cache from the LRU queue held by query for expulsion.
- for (auto iter = query_context->queue().begin(); iter !=
query_context->queue().end(); iter++) {
- if (!is_overflow()) {
- break;
- }
-
- auto* cell = get_cell(iter->hash, iter->offset, cache_lock);
- if (!cell) {
- /// The cache corresponding to this record may be swapped out by
- /// other queries, so it has become invalid.
- ghost.push_back(iter);
- ghost_remove_size += iter->size;
- } else {
- size_t cell_size = cell->size();
- DCHECK(iter->size == cell_size);
-
- if (cell->releasable()) {
- auto& file_block = cell->file_block;
-
- std::lock_guard block_lock(file_block->_mutex);
- DCHECK(file_block->_download_state ==
FileBlock::State::DOWNLOADED);
- to_evict.push_back(cell);
- removed_size += cell_size;
- }
- }
- }
-
- auto remove_file_block_if = [&](FileBlockCell* cell) {
- FileBlockSPtr file_block = cell->file_block;
- if (file_block) {
- query_context->remove(file_block->get_hash_value(),
file_block->offset(), cache_lock);
- std::lock_guard block_lock(file_block->_mutex);
- remove(file_block, cache_lock, block_lock);
- }
- };
-
- for (auto& iter : ghost) {
- query_context->remove(iter->hash, iter->offset, cache_lock);
- }
-
- std::for_each(to_evict.begin(), to_evict.end(), remove_file_block_if);
-
- if (is_overflow() &&
- !try_reserve_from_other_queue(context.cache_type, size, cur_time,
cache_lock)) {
+ bool other_queue_success =
+ config::file_cache_query_limit_enable_evict_from_other_queue
Review Comment:
这里直接用 config 的方式不是最优的。更优雅的处理应该是先判断 cache 空间是否足够，如果 cache 整体有空闲空间，我们应该无条件地让这个
query 使用超额的空间。如果 cache 整体已满，可以再做 config 的判断
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -1018,73 +1035,19 @@ bool BlockFileCache::try_reserve(const UInt128Wrapper&
hash, const CacheContext&
int64_t cur_time = std::chrono::duration_cast<std::chrono::seconds>(
std::chrono::steady_clock::now().time_since_epoch())
.count();
- auto& queue = get_queue(context.cache_type);
- size_t removed_size = 0;
- size_t ghost_remove_size = 0;
- size_t queue_size = queue.get_capacity(cache_lock);
- size_t cur_cache_size = _cur_cache_size;
- size_t query_context_cache_size =
query_context->get_cache_size(cache_lock);
-
- std::vector<LRUQueue::Iterator> ghost;
- std::vector<FileBlockCell*> to_evict;
-
- size_t max_size = queue.get_max_size();
- auto is_overflow = [&] {
- return _disk_resource_limit_mode ? removed_size < size
- : cur_cache_size + size -
removed_size > _capacity ||
- (queue_size + size -
removed_size > max_size) ||
- (query_context_cache_size +
size -
- (removed_size +
ghost_remove_size) >
-
query_context->get_max_cache_size());
- };
-
- /// Select the cache from the LRU queue held by query for expulsion.
- for (auto iter = query_context->queue().begin(); iter !=
query_context->queue().end(); iter++) {
- if (!is_overflow()) {
- break;
- }
-
- auto* cell = get_cell(iter->hash, iter->offset, cache_lock);
- if (!cell) {
- /// The cache corresponding to this record may be swapped out by
- /// other queries, so it has become invalid.
- ghost.push_back(iter);
- ghost_remove_size += iter->size;
- } else {
- size_t cell_size = cell->size();
- DCHECK(iter->size == cell_size);
-
- if (cell->releasable()) {
- auto& file_block = cell->file_block;
-
- std::lock_guard block_lock(file_block->_mutex);
- DCHECK(file_block->_download_state ==
FileBlock::State::DOWNLOADED);
- to_evict.push_back(cell);
- removed_size += cell_size;
- }
- }
- }
-
- auto remove_file_block_if = [&](FileBlockCell* cell) {
- FileBlockSPtr file_block = cell->file_block;
- if (file_block) {
- query_context->remove(file_block->get_hash_value(),
file_block->offset(), cache_lock);
- std::lock_guard block_lock(file_block->_mutex);
- remove(file_block, cache_lock, block_lock);
- }
- };
-
- for (auto& iter : ghost) {
- query_context->remove(iter->hash, iter->offset, cache_lock);
- }
-
- std::for_each(to_evict.begin(), to_evict.end(), remove_file_block_if);
-
- if (is_overflow() &&
- !try_reserve_from_other_queue(context.cache_type, size, cur_time,
cache_lock)) {
+ bool other_queue_success =
+ config::file_cache_query_limit_enable_evict_from_other_queue
+ ? try_reserve_from_other_queue(context.cache_type, size,
cur_time, cache_lock)
+ : false;
+ if (!other_queue_success) {
+ LOG(WARNING) << "Failed to reserve space after exhausting all eviction
strategies: "
+ << "query_limit_enable_evict_from_other_queue="
+ <<
config::file_cache_query_limit_enable_evict_from_other_queue
+ << ", hash=" << hash.to_string() << ", offset=" << offset
<< ", size=" << size;
return false;
Review Comment:
这里 return false -> SKIP_CACHE -> 回退到直接访问remote 可能不太合理。SKIP_CACHE
是最不理想的情况，因为它无法利用 cache 的 1MB 预读功能，导致 remote IO
数量多、查询效率慢，我们应该尽量避免。所以我觉得这里我们还是需要在 query 内部的 LRU 里做淘汰的。
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -1018,73 +1035,19 @@ bool BlockFileCache::try_reserve(const UInt128Wrapper&
hash, const CacheContext&
int64_t cur_time = std::chrono::duration_cast<std::chrono::seconds>(
std::chrono::steady_clock::now().time_since_epoch())
.count();
- auto& queue = get_queue(context.cache_type);
- size_t removed_size = 0;
- size_t ghost_remove_size = 0;
- size_t queue_size = queue.get_capacity(cache_lock);
- size_t cur_cache_size = _cur_cache_size;
- size_t query_context_cache_size =
query_context->get_cache_size(cache_lock);
-
- std::vector<LRUQueue::Iterator> ghost;
- std::vector<FileBlockCell*> to_evict;
-
- size_t max_size = queue.get_max_size();
- auto is_overflow = [&] {
- return _disk_resource_limit_mode ? removed_size < size
- : cur_cache_size + size -
removed_size > _capacity ||
- (queue_size + size -
removed_size > max_size) ||
- (query_context_cache_size +
size -
- (removed_size +
ghost_remove_size) >
-
query_context->get_max_cache_size());
- };
-
- /// Select the cache from the LRU queue held by query for expulsion.
- for (auto iter = query_context->queue().begin(); iter !=
query_context->queue().end(); iter++) {
- if (!is_overflow()) {
- break;
- }
-
- auto* cell = get_cell(iter->hash, iter->offset, cache_lock);
- if (!cell) {
- /// The cache corresponding to this record may be swapped out by
- /// other queries, so it has become invalid.
- ghost.push_back(iter);
- ghost_remove_size += iter->size;
- } else {
- size_t cell_size = cell->size();
- DCHECK(iter->size == cell_size);
-
- if (cell->releasable()) {
- auto& file_block = cell->file_block;
-
- std::lock_guard block_lock(file_block->_mutex);
- DCHECK(file_block->_download_state ==
FileBlock::State::DOWNLOADED);
- to_evict.push_back(cell);
- removed_size += cell_size;
- }
- }
- }
-
- auto remove_file_block_if = [&](FileBlockCell* cell) {
- FileBlockSPtr file_block = cell->file_block;
- if (file_block) {
- query_context->remove(file_block->get_hash_value(),
file_block->offset(), cache_lock);
- std::lock_guard block_lock(file_block->_mutex);
- remove(file_block, cache_lock, block_lock);
- }
- };
-
- for (auto& iter : ghost) {
- query_context->remove(iter->hash, iter->offset, cache_lock);
- }
-
- std::for_each(to_evict.begin(), to_evict.end(), remove_file_block_if);
-
- if (is_overflow() &&
- !try_reserve_from_other_queue(context.cache_type, size, cur_time,
cache_lock)) {
+ bool other_queue_success =
+ config::file_cache_query_limit_enable_evict_from_other_queue
+ ? try_reserve_from_other_queue(context.cache_type, size,
cur_time, cache_lock)
+ : false;
+ if (!other_queue_success) {
+ LOG(WARNING) << "Failed to reserve space after exhausting all eviction
strategies: "
+ << "query_limit_enable_evict_from_other_queue="
+ <<
config::file_cache_query_limit_enable_evict_from_other_queue
+ << ", hash=" << hash.to_string() << ", offset=" << offset
<< ", size=" << size;
return false;
Review Comment:
换句话说，这里上面删掉的代码，应该是值得保留的。
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -287,6 +301,7 @@ BlockFileCache::QueryFileCacheContextPtr
BlockFileCache::get_query_context(
}
void BlockFileCache::remove_query_context(const TUniqueId& query_id) {
+ LOG(INFO) << "BlockFileCache::remove_query_context called for query_id: "
<< print_id(query_id);
Review Comment:
DEBUG 日志，删掉或者改用 VLOG
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -1018,73 +1035,19 @@ bool BlockFileCache::try_reserve(const UInt128Wrapper&
hash, const CacheContext&
int64_t cur_time = std::chrono::duration_cast<std::chrono::seconds>(
std::chrono::steady_clock::now().time_since_epoch())
.count();
- auto& queue = get_queue(context.cache_type);
- size_t removed_size = 0;
- size_t ghost_remove_size = 0;
- size_t queue_size = queue.get_capacity(cache_lock);
- size_t cur_cache_size = _cur_cache_size;
- size_t query_context_cache_size =
query_context->get_cache_size(cache_lock);
-
- std::vector<LRUQueue::Iterator> ghost;
- std::vector<FileBlockCell*> to_evict;
-
- size_t max_size = queue.get_max_size();
- auto is_overflow = [&] {
- return _disk_resource_limit_mode ? removed_size < size
- : cur_cache_size + size -
removed_size > _capacity ||
- (queue_size + size -
removed_size > max_size) ||
- (query_context_cache_size +
size -
- (removed_size +
ghost_remove_size) >
-
query_context->get_max_cache_size());
- };
-
- /// Select the cache from the LRU queue held by query for expulsion.
- for (auto iter = query_context->queue().begin(); iter !=
query_context->queue().end(); iter++) {
- if (!is_overflow()) {
- break;
- }
-
- auto* cell = get_cell(iter->hash, iter->offset, cache_lock);
- if (!cell) {
- /// The cache corresponding to this record may be swapped out by
- /// other queries, so it has become invalid.
- ghost.push_back(iter);
- ghost_remove_size += iter->size;
- } else {
- size_t cell_size = cell->size();
- DCHECK(iter->size == cell_size);
-
- if (cell->releasable()) {
- auto& file_block = cell->file_block;
-
- std::lock_guard block_lock(file_block->_mutex);
- DCHECK(file_block->_download_state ==
FileBlock::State::DOWNLOADED);
- to_evict.push_back(cell);
- removed_size += cell_size;
- }
- }
- }
-
- auto remove_file_block_if = [&](FileBlockCell* cell) {
- FileBlockSPtr file_block = cell->file_block;
- if (file_block) {
- query_context->remove(file_block->get_hash_value(),
file_block->offset(), cache_lock);
- std::lock_guard block_lock(file_block->_mutex);
- remove(file_block, cache_lock, block_lock);
- }
- };
-
- for (auto& iter : ghost) {
- query_context->remove(iter->hash, iter->offset, cache_lock);
- }
-
- std::for_each(to_evict.begin(), to_evict.end(), remove_file_block_if);
-
- if (is_overflow() &&
- !try_reserve_from_other_queue(context.cache_type, size, cur_time,
cache_lock)) {
+ bool other_queue_success =
+ config::file_cache_query_limit_enable_evict_from_other_queue
+ ? try_reserve_from_other_queue(context.cache_type, size,
cur_time, cache_lock)
+ : false;
+ if (!other_queue_success) {
+ LOG(WARNING) << "Failed to reserve space after exhausting all eviction
strategies: "
Review Comment:
这个日志可能会比较多，可以用 LOG_EVERY_N 优化一下，或者干脆用 bvar Adder 做个数量上的监控就行
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
