matanper commented on code in PR #69290:
URL: https://github.com/apache/doris/pull/69290#discussion_r4522477223
##########
cloud/src/recycler/checker.cpp:
##########
@@ -1049,20 +1063,54 @@ int InstanceChecker::do_inverted_check() {
// so we choose to skip here.
TEST_SYNC_POINT_RETURN_WITH_VALUE("InstanceChecker::do_inverted_check",
(int)0);
- for (auto& [_, accessor] : accessor_map_) {
+ // List each storage location once, resources sharing one would count its
leaks repeatedly.
+ // Compare `location()`, not the normalized `uri()` ("a/b" and "a//b" hold
different objects).
+ std::unordered_set<std::string> checked_locations;
+ for (auto& [resource_id, accessor] : accessor_map_) {
+ if (!checked_locations.insert(accessor->location()).second) {
+ LOG(INFO) << "skip inverted check of a storage location already
checked, instance_id="
+ << instance_id_ << " resource_id=" << resource_id
+ << " uri=" << accessor->uri();
+ continue;
+ }
+ const StorageRoot& root = accessor->root();
+ // Another vault root may be nested in the listed data, e.g. prefix
"root/data" in "root"
+ // is listed here as data/data/...: skip it, it is checked when its
own vault is listed
+ std::string listed_dir = root.prefix.empty() ? "data/" : root.prefix +
"/data/";
+ std::vector<std::string> nested_dirs;
+ for (auto& [_, other] : accessor_map_) {
+ const StorageRoot& other_root = other->root();
+ std::string other_dir = other_root.prefix + "/data/";
+ if (other_root.container == root.container && other_root.prefix !=
root.prefix &&
+ other_dir.starts_with(listed_dir)) {
+ nested_dirs.push_back("data/" +
other_dir.substr(listed_dir.size()));
+ }
+ }
Review Comment:
Done.
It was raised by the reviewer bot as P2, so maybe it's worth adding
verification for vaults they are not sharing paths
##########
cloud/src/recycler/checker.cpp:
##########
@@ -1049,20 +1063,54 @@ int InstanceChecker::do_inverted_check() {
// so we choose to skip here.
TEST_SYNC_POINT_RETURN_WITH_VALUE("InstanceChecker::do_inverted_check",
(int)0);
- for (auto& [_, accessor] : accessor_map_) {
+ // List each storage location once, resources sharing one would count its
leaks repeatedly.
+ // Compare `location()`, not the normalized `uri()` ("a/b" and "a//b" hold
different objects).
+ std::unordered_set<std::string> checked_locations;
+ for (auto& [resource_id, accessor] : accessor_map_) {
+ if (!checked_locations.insert(accessor->location()).second) {
+ LOG(INFO) << "skip inverted check of a storage location already
checked, instance_id="
+ << instance_id_ << " resource_id=" << resource_id
+ << " uri=" << accessor->uri();
+ continue;
+ }
+ const StorageRoot& root = accessor->root();
+ // Another vault root may be nested in the listed data, e.g. prefix
"root/data" in "root"
+ // is listed here as data/data/...: skip it, it is checked when its
own vault is listed
+ std::string listed_dir = root.prefix.empty() ? "data/" : root.prefix +
"/data/";
+ std::vector<std::string> nested_dirs;
+ for (auto& [_, other] : accessor_map_) {
+ const StorageRoot& other_root = other->root();
+ std::string other_dir = other_root.prefix + "/data/";
+ if (other_root.container == root.container && other_root.prefix !=
root.prefix &&
+ other_dir.starts_with(listed_dir)) {
+ nested_dirs.push_back("data/" +
other_dir.substr(listed_dir.size()));
+ }
+ }
Review Comment:
Done.
It was raised by the reviewer bot as P2, so maybe it's worth adding
verification for vaults they are not sharing paths
--
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]
