Copilot commented on code in PR #63344:
URL: https://github.com/apache/doris/pull/63344#discussion_r3256991994
##########
be/src/io/cache/fs_file_cache_storage.cpp:
##########
@@ -281,25 +281,33 @@ 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()) {
+ RETURN_IF_ERROR(fs->delete_directory(v2_dir));
+ }
+
+ files.clear();
+ exists = false;
+ RETURN_IF_ERROR(fs->list(v3_dir, true, &files, &exists));
if (files.empty()) {
- RETURN_IF_ERROR(fs->delete_directory(dir));
+ RETURN_IF_ERROR(fs->delete_directory(v3_dir));
Review Comment:
`fs->delete_directory()` is recursive for the local filesystem
(`LocalFileSystem::delete_directory_impl` calls `std::filesystem::remove_all`),
so this can delete a new block/tmp file that is created in `v3_dir` after the
`list()` emptiness check. Use a non-recursive rmdir-style removal (for example
`std::filesystem::remove` and ignore ENOTEMPTY/NOT_FOUND) for empty-directory
cleanup so concurrent cache writes are not removed.
##########
be/src/io/cache/fs_file_cache_storage.cpp:
##########
@@ -281,25 +281,33 @@ 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()) {
+ RETURN_IF_ERROR(fs->delete_directory(v2_dir));
+ }
+
+ files.clear();
+ exists = false;
+ RETURN_IF_ERROR(fs->list(v3_dir, true, &files, &exists));
if (files.empty()) {
- RETURN_IF_ERROR(fs->delete_directory(dir));
+ RETURN_IF_ERROR(fs->delete_directory(v3_dir));
Review Comment:
The PR description says the empty v3 prefix directory is also removed, but
the implementation only deletes `v3_dir` and leaves its parent `<hash_prefix>`
directory behind when the last key under that prefix is removed. Add a
non-recursive cleanup for `std::filesystem::path(v3_dir).parent_path()` (and
cover it in the test) or update the scope if prefix cleanup is intentionally
excluded.
--
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]
