github-actions[bot] commented on code in PR #65174:
URL: https://github.com/apache/doris/pull/65174#discussion_r3594092378
##########
be/src/io/cache/block_file_cache_factory.cpp:
##########
@@ -113,13 +105,112 @@ Status FileCacheFactory::create_file_cache(const
std::string& cache_base_path,
<< " total_size: " << file_cache_settings.capacity
<< " disk_total_size: " << disk_capacity;
}
+
auto cache = std::make_unique<BlockFileCache>(cache_base_path,
file_cache_settings);
RETURN_IF_ERROR(cache->initialize());
+ built_cache->cache_base_path = cache_base_path;
+ built_cache->settings = file_cache_settings;
+ built_cache->cache = std::move(cache);
+ return Status::OK();
+}
+
+} // namespace
+
+FileCacheFactory* FileCacheFactory::instance() {
+ return ExecEnv::GetInstance()->file_cache_factory();
+}
+
+size_t FileCacheFactory::try_release() {
+ int elements = 0;
+ for (auto& cache : _caches) {
+ elements += cache->try_release();
+ }
+ return elements;
+}
+
+size_t FileCacheFactory::try_release(const std::string& base_path) {
+ auto iter = _path_to_cache.find(base_path);
+ if (iter != _path_to_cache.end()) {
+ return iter->second->try_release();
+ }
+ return 0;
+}
+
+Status FileCacheFactory::create_file_cache(const std::string& cache_base_path,
+ FileCacheSettings
file_cache_settings) {
+ BuiltFileCache built_cache;
+ RETURN_IF_ERROR(build_file_cache(cache_base_path, file_cache_settings,
&built_cache));
+ {
+ std::lock_guard lock(_mtx);
+ _path_to_cache[built_cache.cache_base_path] = built_cache.cache.get();
+ _capacity += built_cache.settings.capacity;
+ _caches.push_back(std::move(built_cache.cache));
+ }
+
+ return Status::OK();
+}
+
+Status FileCacheFactory::create_file_caches(
+ const std::vector<CachePath>& cache_paths,
+ const std::function<bool(const std::string&, const Status&)>&
should_ignore_error) {
+ struct BuildResult {
+ std::string cache_base_path;
+ FileCacheSettings settings;
+ BuiltFileCache built_cache;
+ Status status;
+ bool skip = false;
+ };
+
+ std::vector<BuildResult> results;
+ results.reserve(cache_paths.size());
+ std::unordered_set<std::string> cache_path_set;
+ for (const auto& cache_path : cache_paths) {
+ if (cache_path_set.find(cache_path.path) != cache_path_set.end()) {
+ LOG(WARNING) << fmt::format("cache path {} is duplicate",
cache_path.path);
+ continue;
+ }
+
+ cache_path_set.emplace(cache_path.path);
+ auto& result = results.emplace_back();
+ result.cache_base_path = cache_path.path;
+ result.settings = cache_path.init_settings();
+ }
+
+ std::vector<std::thread> workers;
+ workers.reserve(results.size());
+ for (auto& result : results) {
+ auto* result_ptr = &result;
+ workers.emplace_back([result_ptr]() {
Review Comment:
[P2] Keep the time conversion out of the worker fan-out — Each worker
constructs `BlockFileCache`, which constructs `CacheLRUDumper`; that
constructor calls `std::localtime` and formats the returned static `tm*`. With
two cache paths, one call can overwrite that object while the other worker is
in `std::put_time`. POSIX explicitly permits simultaneous `localtime` calls to
be non-thread-safe, while the old startup loop constructed these objects
serially. This is undefined behavior and can corrupt the `_start_time` used for
first-dump backup names. Please use `localtime_r` with a stack `tm` (or
construct this state before fan-out).
##########
be/src/io/cache/block_file_cache_factory.cpp:
##########
@@ -113,13 +105,112 @@ Status FileCacheFactory::create_file_cache(const
std::string& cache_base_path,
<< " total_size: " << file_cache_settings.capacity
<< " disk_total_size: " << disk_capacity;
}
+
auto cache = std::make_unique<BlockFileCache>(cache_base_path,
file_cache_settings);
RETURN_IF_ERROR(cache->initialize());
+ built_cache->cache_base_path = cache_base_path;
+ built_cache->settings = file_cache_settings;
+ built_cache->cache = std::move(cache);
+ return Status::OK();
+}
+
+} // namespace
+
+FileCacheFactory* FileCacheFactory::instance() {
+ return ExecEnv::GetInstance()->file_cache_factory();
+}
+
+size_t FileCacheFactory::try_release() {
+ int elements = 0;
+ for (auto& cache : _caches) {
+ elements += cache->try_release();
+ }
+ return elements;
+}
+
+size_t FileCacheFactory::try_release(const std::string& base_path) {
+ auto iter = _path_to_cache.find(base_path);
+ if (iter != _path_to_cache.end()) {
+ return iter->second->try_release();
+ }
+ return 0;
+}
+
+Status FileCacheFactory::create_file_cache(const std::string& cache_base_path,
+ FileCacheSettings
file_cache_settings) {
+ BuiltFileCache built_cache;
+ RETURN_IF_ERROR(build_file_cache(cache_base_path, file_cache_settings,
&built_cache));
+ {
+ std::lock_guard lock(_mtx);
+ _path_to_cache[built_cache.cache_base_path] = built_cache.cache.get();
+ _capacity += built_cache.settings.capacity;
+ _caches.push_back(std::move(built_cache.cache));
+ }
+
+ return Status::OK();
+}
+
+Status FileCacheFactory::create_file_caches(
+ const std::vector<CachePath>& cache_paths,
+ const std::function<bool(const std::string&, const Status&)>&
should_ignore_error) {
+ struct BuildResult {
+ std::string cache_base_path;
+ FileCacheSettings settings;
+ BuiltFileCache built_cache;
+ Status status;
+ bool skip = false;
+ };
+
+ std::vector<BuildResult> results;
+ results.reserve(cache_paths.size());
+ std::unordered_set<std::string> cache_path_set;
+ for (const auto& cache_path : cache_paths) {
+ if (cache_path_set.find(cache_path.path) != cache_path_set.end()) {
Review Comment:
[P2] Serialize destructive preparation of overlapping cache roots — This set
removes only byte-for-byte duplicate paths, while `parse_conf_cache_paths`
accepts ancestor/descendant roots. With `clear_file_cache=true` and a
parent-first config such as `/var/lib/doris/cache` plus
`/var/lib/doris/cache/sub`, the child worker can recreate and start
initializing `sub` before the parent worker's recursive
`delete_directory(parent)` removes it. The old serial loop completed the parent
clear before the child began. Please canonicalize and reject overlapping roots,
or perform the destructive clear/create stage serially before parallel cache
initialization.
##########
be/src/io/cache/cache_lru_dumper.cpp:
##########
@@ -431,20 +433,16 @@ Status CacheLRUDumper::parse_one_lru_entry(std::ifstream&
in, std::string& filen
LOG(WARNING) << warn_msg;
return Status::InternalError(warn_msg);
}
-
- // Remove processed group info
-
_parse_meta.mutable_group_offset_size()->erase(_parse_meta.group_offset_size().begin());
+ _parse_entry_index = 0;
}
// Get next entry from current group
VLOG_DEBUG << "After deserialization: " <<
_current_parse_group.DebugString();
- auto entry = _current_parse_group.entries(0);
+ const auto& entry = _current_parse_group.entries(_parse_entry_index++);
Review Comment:
[P2] Release the startup-only protobuf parse storage — The cursor removes
the quadratic erases, but `ParseFromString` clears protobuf messages for reuse
without freeing their allocations. After a full 10,000-entry group, each cache
therefore retains those entry objects (including nested hashes) in
`_current_parse_group` for the entire `BlockFileCache` lifetime, even though
parsing ends after the four startup restores; the old per-entry erase deleted
them. Please swap/destroy the parse messages once restore finishes, including
error exits, or make the parse state local to the restore session.
##########
be/src/io/cache/block_file_cache_factory.cpp:
##########
@@ -113,13 +105,112 @@ Status FileCacheFactory::create_file_cache(const
std::string& cache_base_path,
<< " total_size: " << file_cache_settings.capacity
<< " disk_total_size: " << disk_capacity;
}
+
auto cache = std::make_unique<BlockFileCache>(cache_base_path,
file_cache_settings);
RETURN_IF_ERROR(cache->initialize());
+ built_cache->cache_base_path = cache_base_path;
+ built_cache->settings = file_cache_settings;
+ built_cache->cache = std::move(cache);
+ return Status::OK();
+}
+
+} // namespace
+
+FileCacheFactory* FileCacheFactory::instance() {
+ return ExecEnv::GetInstance()->file_cache_factory();
+}
+
+size_t FileCacheFactory::try_release() {
+ int elements = 0;
+ for (auto& cache : _caches) {
+ elements += cache->try_release();
+ }
+ return elements;
+}
+
+size_t FileCacheFactory::try_release(const std::string& base_path) {
+ auto iter = _path_to_cache.find(base_path);
+ if (iter != _path_to_cache.end()) {
+ return iter->second->try_release();
+ }
+ return 0;
+}
+
+Status FileCacheFactory::create_file_cache(const std::string& cache_base_path,
+ FileCacheSettings
file_cache_settings) {
+ BuiltFileCache built_cache;
+ RETURN_IF_ERROR(build_file_cache(cache_base_path, file_cache_settings,
&built_cache));
+ {
+ std::lock_guard lock(_mtx);
+ _path_to_cache[built_cache.cache_base_path] = built_cache.cache.get();
+ _capacity += built_cache.settings.capacity;
+ _caches.push_back(std::move(built_cache.cache));
+ }
+
+ return Status::OK();
+}
+
+Status FileCacheFactory::create_file_caches(
+ const std::vector<CachePath>& cache_paths,
+ const std::function<bool(const std::string&, const Status&)>&
should_ignore_error) {
+ struct BuildResult {
+ std::string cache_base_path;
+ FileCacheSettings settings;
+ BuiltFileCache built_cache;
+ Status status;
+ bool skip = false;
+ };
+
+ std::vector<BuildResult> results;
+ results.reserve(cache_paths.size());
+ std::unordered_set<std::string> cache_path_set;
+ for (const auto& cache_path : cache_paths) {
+ if (cache_path_set.find(cache_path.path) != cache_path_set.end()) {
+ LOG(WARNING) << fmt::format("cache path {} is duplicate",
cache_path.path);
+ continue;
+ }
+
+ cache_path_set.emplace(cache_path.path);
+ auto& result = results.emplace_back();
+ result.cache_base_path = cache_path.path;
+ result.settings = cache_path.init_settings();
+ }
+
+ std::vector<std::thread> workers;
+ workers.reserve(results.size());
+ for (auto& result : results) {
+ auto* result_ptr = &result;
+ workers.emplace_back([result_ptr]() {
+ SCOPED_INIT_THREAD_CONTEXT();
+ result_ptr->status = build_file_cache(result_ptr->cache_base_path,
result_ptr->settings,
+ &result_ptr->built_cache);
+ });
+ }
+
+ for (auto& worker : workers) {
Review Comment:
[P2] Stop later cache work after a fatal path error — Statuses are not
inspected until every worker has joined. If the first configured disk fails
with `ignore_broken_disk=false`, a later healthy path can still spend minutes
synchronously restoring a 5M-entry LRU dump; returning the known fatal error
then destroys that cache and waits for its uncancellable async metadata loader
as well. The previous ordered loop failed before touching later paths. Please
add cooperative cancellation/prevalidation so a non-ignored error stops later
heavy initialization, and cover this ordering with a failure-path test.
##########
be/src/io/cache/block_file_cache_factory.cpp:
##########
@@ -113,13 +105,112 @@ Status FileCacheFactory::create_file_cache(const
std::string& cache_base_path,
<< " total_size: " << file_cache_settings.capacity
<< " disk_total_size: " << disk_capacity;
}
+
auto cache = std::make_unique<BlockFileCache>(cache_base_path,
file_cache_settings);
RETURN_IF_ERROR(cache->initialize());
+ built_cache->cache_base_path = cache_base_path;
+ built_cache->settings = file_cache_settings;
+ built_cache->cache = std::move(cache);
+ return Status::OK();
+}
+
+} // namespace
+
+FileCacheFactory* FileCacheFactory::instance() {
+ return ExecEnv::GetInstance()->file_cache_factory();
+}
+
+size_t FileCacheFactory::try_release() {
+ int elements = 0;
+ for (auto& cache : _caches) {
+ elements += cache->try_release();
+ }
+ return elements;
+}
+
+size_t FileCacheFactory::try_release(const std::string& base_path) {
+ auto iter = _path_to_cache.find(base_path);
+ if (iter != _path_to_cache.end()) {
+ return iter->second->try_release();
+ }
+ return 0;
+}
+
+Status FileCacheFactory::create_file_cache(const std::string& cache_base_path,
+ FileCacheSettings
file_cache_settings) {
+ BuiltFileCache built_cache;
+ RETURN_IF_ERROR(build_file_cache(cache_base_path, file_cache_settings,
&built_cache));
+ {
+ std::lock_guard lock(_mtx);
+ _path_to_cache[built_cache.cache_base_path] = built_cache.cache.get();
+ _capacity += built_cache.settings.capacity;
+ _caches.push_back(std::move(built_cache.cache));
+ }
+
+ return Status::OK();
+}
+
+Status FileCacheFactory::create_file_caches(
+ const std::vector<CachePath>& cache_paths,
+ const std::function<bool(const std::string&, const Status&)>&
should_ignore_error) {
+ struct BuildResult {
+ std::string cache_base_path;
+ FileCacheSettings settings;
+ BuiltFileCache built_cache;
+ Status status;
+ bool skip = false;
+ };
+
+ std::vector<BuildResult> results;
+ results.reserve(cache_paths.size());
+ std::unordered_set<std::string> cache_path_set;
+ for (const auto& cache_path : cache_paths) {
+ if (cache_path_set.find(cache_path.path) != cache_path_set.end()) {
+ LOG(WARNING) << fmt::format("cache path {} is duplicate",
cache_path.path);
+ continue;
+ }
+
+ cache_path_set.emplace(cache_path.path);
+ auto& result = results.emplace_back();
+ result.cache_base_path = cache_path.path;
+ result.settings = cache_path.init_settings();
+ }
+
+ std::vector<std::thread> workers;
+ workers.reserve(results.size());
+ for (auto& result : results) {
+ auto* result_ptr = &result;
+ workers.emplace_back([result_ptr]() {
Review Comment:
[P2] Avoid concurrent bvar exposure for colliding path prefixes —
`BlockFileCache` exposes its metrics with the cache path as the prefix, and
pinned brpc 1.4.0 normalizes punctuation to `_`. Thus distinct roots such as
`/mnt/cache-a`, `/mnt/cache.a`, and `/mnt/cache_a` have identical bvar names.
Parallel construction can split successive metrics across different cache
objects; with three colliding prefixes, the two losing exposes can also enter
duplicate-name handling concurrently and race on brpc's non-atomic
`s_bvar_may_abort` outside the registry locks. The old loop made ownership
deterministic by constructing caches serially. Please use a collision-free
prefix/reject normalized collisions, or keep metric-bearing construction
outside the fan-out.
--
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]
