github-actions[bot] commented on code in PR #63344:
URL: https://github.com/apache/doris/pull/63344#discussion_r3279181762
##########
be/src/io/cache/fs_file_cache_storage.cpp:
##########
@@ -281,25 +281,39 @@ Status FSFileCacheStorage::read(const FileCacheKey& key,
size_t value_offset, Sl
}
Status FSFileCacheStorage::remove(const FileCacheKey& key) {
- std::string dir = get_path_in_local_cache_v3(key.hash);
- std::string file = get_path_in_local_cache_v3(dir, key.offset);
+ const std::string v3_dir = get_path_in_local_cache_v3(key.hash);
+ const std::string v3_file = get_path_in_local_cache_v3(v3_dir, key.offset);
FDCache::instance()->remove_file_reader(std::make_pair(key.hash,
key.offset));
- RETURN_IF_ERROR(fs->delete_file(file));
+ RETURN_IF_ERROR(fs->delete_file(v3_file));
// return OK not means the file is deleted, it may be not exist
+ std::string v2_dir;
{ // try to detect the file with old v2 format
- dir = get_path_in_local_cache_v2(key.hash, key.meta.expiration_time);
- file = get_path_in_local_cache_v2(dir, key.offset, key.meta.type);
- RETURN_IF_ERROR(fs->delete_file(file));
+ v2_dir = get_path_in_local_cache_v2(key.hash,
key.meta.expiration_time);
+ const std::string v2_file = get_path_in_local_cache_v2(v2_dir,
key.offset, key.meta.type);
+ RETURN_IF_ERROR(fs->delete_file(v2_file));
}
BlockMetaKey mkey(key.meta.tablet_id, UInt128Wrapper(key.hash),
key.offset);
_meta_store->delete_key(mkey);
std::vector<FileInfo> files;
bool exists {false};
- RETURN_IF_ERROR(fs->list(dir, true, &files, &exists));
+ RETURN_IF_ERROR(fs->list(v2_dir, true, &files, &exists));
+ if (files.empty()) {
+ auto st = fs->delete_empty_directory(v2_dir);
Review Comment:
This now calls `delete_empty_directory(v2_dir)` whenever `files.empty()`,
but `list()` returns `Status::OK()` with `exists=false` and an empty `files`
vector when the legacy v2 directory is absent. For a normal v3-only cache
block, `delete_file(v2_file)` is OK for the missing legacy file, `list(v2_dir,
..., &exists)` sets `exists` to false, and this `rmdir` returns `NOT_FOUND`, so
every such removal logs `failed to remove cache directory`. Please gate the
cleanup on `exists && files.empty()` (or make the empty-directory cleanup treat
missing directories as success) and add a test for removing a v3 block with no
v2 directory. This is distinct from the existing v3 race thread because it is
the deterministic legacy-v2 path, not a concurrent v3 cleanup race.
--
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]
