github-actions[bot] commented on code in PR #64186:
URL: https://github.com/apache/doris/pull/64186#discussion_r3409266115
##########
be/src/io/cache/fs_file_cache_storage.cpp:
##########
@@ -672,13 +881,70 @@ bool FSFileCacheStorage::handle_already_loaded_block(
return false;
}
auto block = cell_it->second.file_block;
+ if (is_tmp) {
+ return true;
+ }
+ bool accepted_storage_path = true;
+ if (!storage_file_exists(block->storage_key())) {
+ block->update_storage_expiration_time(context.storage_expiration());
+ block->update_storage_cache_type(context.cache_type);
+ } else if (block->storage_expiration_time() !=
context.storage_expiration()) {
+ remove_file_and_empty_dir(offset_path, key_path);
+ accepted_storage_path = false;
+ } else if (block->storage_cache_type() != context.cache_type) {
+ block->update_storage_cache_type(context.cache_type);
+ }
size_t old_size = block->range().size();
- if (old_size != new_size) {
+ if (accepted_storage_path && old_size != new_size) {
mgr->reset_range(hash, offset, old_size, new_size, cache_lock);
}
+ if (accepted_storage_path && (block->cache_type() != context.cache_type ||
+ block->expiration_time() !=
context.expiration_time)) {
+ mgr->update_hash_logical_meta(hash, context.cache_type,
context.expiration_time,
+ cache_lock);
+ }
return true;
}
+void FSFileCacheStorage::load_blocks_from_dir_unlocked(
+ BlockFileCache* mgr, const UInt128Wrapper& hash, const KeyDir& key_dir,
+ const FileCacheKey* logical_key, std::lock_guard<std::mutex>&
cache_lock) const {
+ CacheContext context_original;
+ context_original.query_id = TUniqueId();
+ context_original.expiration_time =
+ logical_key == nullptr ? key_dir.expiration_time :
logical_key->meta.expiration_time;
+ context_original.storage_expiration_time = key_dir.expiration_time;
Review Comment:
This startup path still derives the logical expiration from the storage
directory when `logical_key == nullptr`, which makes the new logical/storage
split non-durable across BE restarts. For example,
`modify_expiration_time(hash, new_expiration)` now intentionally leaves files
under the old `hash_<old>` directory, and `modify_expiration_time(hash, 0)`
does the same when TTL is cleared. After restart,
`load_cache_info_into_memory()` reaches this branch with no logical key,
`parse_filename_suffix_to_cache_type()` forces `TTL` for the positive directory
expiration, and `add_cell()` repopulates `_key_to_time` from the old storage
expiration. The logical update is lost, and a hash whose TTL was cleared is
resurrected as TTL until another metadata sync happens. Please persist/replay
the logical expiration separately, or avoid treating a storage-only directory
suffix as authoritative logical TTL during startup load. This is distinct from
the existing absent-hash `_key_to_time` thread bec
ause it affects hashes whose logical metadata was validly changed before
restart.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -1378,23 +1553,34 @@ void BlockFileCache::remove_if_cached(const
UInt128Wrapper& file_key) {
// if in use, cache meta will be deleted after use and the block file is then
deleted asynchronously
void BlockFileCache::remove_if_cached_async(const UInt128Wrapper& file_key) {
std::string reason = "remove_if_cached_async";
- SCOPED_CACHE_LOCK(_mutex, this);
- bool is_ttl_file = remove_if_ttl_file_blocks(file_key, true, cache_lock,
/*sync*/ false);
- if (!is_ttl_file) {
- auto iter = _files.find(file_key);
- std::vector<FileBlockCell*> to_remove;
- if (iter != _files.end()) {
- for (auto& [_, cell] : iter->second) {
- *_gc_evict_bytes_metrics << cell.size();
- *_gc_evict_count_metrics << 1;
- if (cell.releasable()) {
- to_remove.push_back(&cell);
- } else {
- cell.file_block->set_deleting();
+ bool has_unreleasable = false;
+ {
+ SCOPED_CACHE_LOCK(_mutex, this);
+ bool is_ttl_file = remove_if_ttl_file_blocks(file_key, true,
cache_lock, /*sync*/ false,
+ &has_unreleasable);
+ if (!is_ttl_file) {
+ auto iter = _files.find(file_key);
+ std::vector<FileBlockCell*> to_remove;
+ if (iter != _files.end()) {
+ for (auto& [_, cell] : iter->second) {
+ *_gc_evict_bytes_metrics << cell.size();
+ *_gc_evict_count_metrics << 1;
+ if (cell.releasable()) {
+ to_remove.push_back(&cell);
+ } else {
+ has_unreleasable = true;
+ cell.file_block->set_deleting();
+ }
}
}
+ remove_file_blocks(to_remove, cache_lock, false, reason);
}
- remove_file_blocks(to_remove, cache_lock, false, reason);
+ if (has_unreleasable) {
+ _hashes_pending_cleanup.insert(file_key);
+ }
+ }
+ if (!has_unreleasable) {
+ enqueue_hash_for_cleanup(file_key);
}
Review Comment:
Queuing a hash-wide cleanup after releasing `_mutex` can delete cache files
created after this invalidation. `remove_if_cached_async()` removes the current
in-memory cells while holding the cache lock, then a later `get_or_set()` for
the same hash can add a new live cell and write a new file before the GC thread
drains `_recycle_hashes`; when `remove_all_by_hash()` eventually runs it
removes every `hash_*` directory, including that new file, leaving the live
cell pointing at missing storage. The synchronous `remove_if_cached()` has the
same post-lock window around its direct `remove_all_by_hash()` call. Please
scope cleanup to the directories observed during invalidation, or otherwise
coordinate hash-wide deletion with new cell creation for the same hash.
--
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]
