github-actions[bot] commented on code in PR #69290:
URL: https://github.com/apache/doris/pull/69290#discussion_r4346909331
##########
cloud/src/recycler/checker.cpp:
##########
@@ -991,6 +990,11 @@ int InstanceChecker::do_inverted_check() {
}
}
}
+ if (stopped()) {
+ // The loop above stops early when the checker is stopped, the
rowsets are incomplete
+ return -1;
Review Comment:
[P2] Stop object traversal when the checker is canceled
If shutdown or lease loss sets `stopped_` during a tablet load, this returns
only from `check_segment_file` and leaves `tablet_rowsets_cache.tablet_id ==
0`. The enclosing object loop keeps going; each remaining `.dat` for that same
tablet then creates a new transaction and executes the first range read because
`it == nullptr` makes the read loop run even when stopped. Previously the
eagerly assigned tablet id let those subsequent objects reuse the cache, so
this introduces a metadata read per remaining segment after cancellation. On a
large tablet this can add thousands of FDB reads while `Checker::stop()` waits
in `join()`, or after the worker has lost its lease. Propagate cancellation to
the outer traversal before starting any further metadata work.
##########
cloud/src/recycler/checker.cpp:
##########
@@ -1049,7 +1053,19 @@ 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_) {
+ // Several resources may be backed by the same storage location, e.g.
storage vaults on the same
+ // bucket and prefix with different credentials. List each location only
once, otherwise its
+ // objects would be scanned and their leaks counted repeatedly. Compare
the location as
+ // configured, not the normalized `uri()`: prefixes "a/b" and "a//b" share
a uri but hold
+ // different objects, and skipping one would hide its leaks.
+ std::unordered_set<std::string> checked_locations;
+ for (auto& [resource_id, accessor] : accessor_map_) {
+ if (!checked_locations.insert(accessor->location()).second) {
Review Comment:
[P2] Give HDFS vaults their actual storage location
For two HDFS vaults with different prefixes or namenodes, this condition
still skips one: `HdfsAccessor` inherits `location()`, which returns
`StorageVaultAccessor::uri_`, but its constructor initializes a separate,
shadowing `HdfsAccessor::uri_` (`hdfs_accessor.h:83`, `hdfs_accessor.cpp:333`).
Both accessors therefore return the empty string. Only the first vault is
listed, so leaks in all other HDFS vaults remain invisible and the check can
return success. This is distinct from the previously reported S3
normalized-prefix collision, which the new S3 override fixes. Provide an HDFS
location implementation (or populate the base member) and cover distinct HDFS
vaults.
--
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]
